Re: [PATCH] drm: rcar-du: Add r8a774c0 device support
Hi Fabrizio,
Thank you for the patch.
On Thursday, 13 December 2018 22:23:27 EET Fabrizio Castro wrote:
> Add support for the RZ/G2E (R8A774C0) SoC to the R-Car DU driver.
>
> Signed-off-by: Fabrizio Castro
Reviewed-by: Laurent Pinchart
and applied to my tree.
> ---
> drivers/gpu/drm/rcar-du/rcar_du_drv.c | 28
> 1 file changed, 28 insertions(+)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 9caff39..ac32510 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -104,6 +104,33 @@ static const struct rcar_du_device_info
> rzg1_du_r8a77470_info = { },
> };
>
> +static const struct rcar_du_device_info rcar_du_r8a774c0_info = {
> + .gen = 3,
> + .features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
> + | RCAR_DU_FEATURE_EXT_CTRL_REGS
> + | RCAR_DU_FEATURE_VSP1_SOURCE,
> + .channels_mask = BIT(1) | BIT(0),
> + .routes = {
> + /*
> + * R8A774C0 has one RGB output and two LVDS outputs
> + */
> + [RCAR_DU_OUTPUT_DPAD0] = {
> + .possible_crtcs = BIT(0) | BIT(1),
> + .port = 0,
> + },
> + [RCAR_DU_OUTPUT_LVDS0] = {
> + .possible_crtcs = BIT(0),
> + .port = 1,
> + },
> + [RCAR_DU_OUTPUT_LVDS1] = {
> + .possible_crtcs = BIT(1),
> + .port = 2,
> + },
> + },
> + .num_lvds = 2,
> + .lvds_clk_mask = BIT(1) | BIT(0),
> +};
> +
> static const struct rcar_du_device_info rcar_du_r8a7779_info = {
> .gen = 2,
> .features = RCAR_DU_FEATURE_INTERLACED
> @@ -371,6 +398,7 @@ static const struct of_device_id rcar_du_of_table[] = {
> { .compatible = "renesas,du-r8a7744", .data = &rzg1_du_r8a7743_info },
> { .compatible = "renesas,du-r8a7745", .data = &rzg1_du_r8a7745_info },
> { .compatible = "renesas,du-r8a77470", .data = &rzg1_du_r8a77470_info },
> + { .compatible = "renesas,du-r8a774c0", .data = &rcar_du_r8a774c0_info },
> { .compatible = "renesas,du-r8a7779", .data = &rcar_du_r8a7779_info },
> { .compatible = "renesas,du-r8a7790", .data = &rcar_du_r8a7790_info },
> { .compatible = "renesas,du-r8a7791", .data = &rcar_du_r8a7791_info },
--
Regards,
Laurent Pinchart
Re: [PATCH] dt-bindings: display: renesas: lvds: Document r8a774c0 bindings
Hi Fabrizio, Thank you for the patch. On Thursday, 13 December 2018 22:20:54 EET Fabrizio Castro wrote: > The RZ/G2E (r8a774c0) supports two LVDS channels. Extend the binding to > support them. > > Signed-off-by: Fabrizio Castro Reviewed-by: Laurent Pinchart and applied to my tree. > --- > Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git > a/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt > b/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt index > 3aeb0ec..cb7c328 100644 > --- a/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt > +++ b/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt > @@ -8,6 +8,7 @@ Required properties: > > - compatible : Shall contain one of >- "renesas,r8a7743-lvds" for R8A7743 (RZ/G1M) compatible LVDS encoders > + - "renesas,r8a774c0-lvds" for R8A774C0 (RZ/G2E) compatible LVDS encoders >- "renesas,r8a7790-lvds" for R8A7790 (R-Car H2) compatible LVDS encoders >- "renesas,r8a7791-lvds" for R8A7791 (R-Car M2-W) compatible LVDS > encoders - "renesas,r8a7793-lvds" for R8A7793 (R-Car M2-N) compatible LVDS > encoders @@ -24,7 +25,7 @@ Required properties: > - clock-names: Name of the clocks. This property is model-dependent. >- The functional clock, which mandatory for all models, shall be listed > first, and shall be named "fck". > - - On R8A77990 and R8A77995, the LVDS encoder can use the EXTAL or > + - On R8A77990, R8A77995 and R8A774C0, the LVDS encoder can use the EXTAL > or DU_DOTCLKINx clocks. Those clocks are optional. When supplied they must > be named "extal" and "dclkin.x" respectively, with "x" being the > DU_DOTCLKIN numerical index. -- Regards, Laurent Pinchart
Re: [PATCH] drm: rcar-du: lvds: add R8A774C0 support
Hi Fabrizio,
Thank you for the patch.
On Thursday, 13 December 2018 22:23:36 EET Fabrizio Castro wrote:
> The LVDS implementation on the RZ/G2E (a.k.a. R8A774C0) is very similar
> to the one found on R-Car E3 (a.k.a. R8A77990), therefore add RZ/G2E
> LVDS support to the LVDS encoder driver in a similar fashion to what
s/what/what is/
> done for R-Car E3.
>
> Signed-off-by: Fabrizio Castro
Reviewed-by: Laurent Pinchart
and applied to my tree with the above fix.
> ---
> drivers/gpu/drm/rcar-du/rcar_lvds.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> b/drivers/gpu/drm/rcar-du/rcar_lvds.c index 173d7ad..329dfca 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -785,6 +785,7 @@ static const struct rcar_lvds_device_info
> rcar_lvds_r8a77995_info = {
>
> static const struct of_device_id rcar_lvds_of_table[] = {
> { .compatible = "renesas,r8a7743-lvds", .data = &rcar_lvds_gen2_info },
> + { .compatible = "renesas,r8a774c0-lvds", .data =
> &rcar_lvds_r8a77990_info
> }, { .compatible = "renesas,r8a7790-lvds", .data = &rcar_lvds_r8a7790_info
> }, { .compatible = "renesas,r8a7791-lvds", .data = &rcar_lvds_gen2_info },
> { .compatible = "renesas,r8a7793-lvds", .data = &rcar_lvds_gen2_info },
--
Regards,
Laurent Pinchart
Re: [PATCH] [media] v4l: vsp1: Add RZ/G support
Hi Fabrizio, Thank you for the patch. On Thursday, 13 December 2018 22:20:24 EET Fabrizio Castro wrote: > Document RZ/G1 and RZ/G2 support. > > Signed-off-by: Fabrizio Castro Reviewed-by: Laurent Pinchart And applied to my tree. > --- > Documentation/devicetree/bindings/media/renesas,vsp1.txt | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/media/renesas,vsp1.txt > b/Documentation/devicetree/bindings/media/renesas,vsp1.txt index > 1642701..cd5a955 100644 > --- a/Documentation/devicetree/bindings/media/renesas,vsp1.txt > +++ b/Documentation/devicetree/bindings/media/renesas,vsp1.txt > @@ -2,13 +2,13 @@ > > The VSP is a video processing engine that supports up-/down-scaling, alpha > blending, color space conversion and various other image processing > features. -It can be found in the Renesas R-Car second generation SoCs. > +It can be found in the Renesas R-Car Gen2, R-Car Gen3, RZ/G1, and RZ/G2 > SoCs. > > Required properties: > >- compatible: Must contain one of the following values > -- "renesas,vsp1" for the R-Car Gen2 VSP1 > -- "renesas,vsp2" for the R-Car Gen3 VSP2 > +- "renesas,vsp1" for the R-Car Gen2 and RZ/G1 VSP1 > +- "renesas,vsp2" for the R-Car Gen3 and RZ/G2 VSP2 > >- reg: Base address and length of the registers block for the VSP. >- interrupts: VSP interrupt specifier. -- Regards, Laurent Pinchart
Re: [PATCH] dt-bindings: media: renesas-fcp: Add RZ/G2 support
Hi Fabrizio, Thank you for the patch. On Thursday, 13 December 2018 22:20:33 EET Fabrizio Castro wrote: > Document RZ/G2 support. > > Signed-off-by: Fabrizio Castro Reviewed-by: Laurent Pinchart And applied to my tree. > --- > Documentation/devicetree/bindings/media/renesas,fcp.txt | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/media/renesas,fcp.txt > b/Documentation/devicetree/bindings/media/renesas,fcp.txt index > 3ec9180..79c3739 100644 > --- a/Documentation/devicetree/bindings/media/renesas,fcp.txt > +++ b/Documentation/devicetree/bindings/media/renesas,fcp.txt > @@ -2,8 +2,9 @@ Renesas R-Car Frame Compression Processor (FCP) > --- > > The FCP is a companion module of video processing modules in the Renesas > R-Car -Gen3 SoCs. It provides data compression and decompression, data > caching, and -conversion of AXI transactions in order to reduce the memory > bandwidth. +Gen3 and RZ/G2 SoCs. It provides data compression and > decompression, data +caching, and conversion of AXI transactions in order > to reduce the memory +bandwidth. > > There are three types of FCP: FCP for Codec (FCPC), FCP for VSP (FCPV) and > FCP for FDP (FCPF). Their configuration and behaviour depend on the module > they -- Regards, Laurent Pinchart
Re: [PATCH V3] clk: vc5: Add suspend/resume support
Hi Marek,
Thank you for the patch.
On Thursday, 13 December 2018 18:15:28 EET Marek Vasut wrote:
> Add simple suspend/resume handlers to the driver to restore the chip
> configuration after resume. It is possible that the chip was configured
> with non-default values before suspend-resume cycle and that the chip
> is powered down during this cycle, so the configuration could get lost.
>
> Signed-off-by: Marek Vasut
> Cc: Alexey Firago
> Cc: Laurent Pinchart
> Cc: Michael Turquette
> Cc: Stephen Boyd
> Cc: linux-renesas-soc@vger.kernel.org
Reviewed-by: Laurent Pinchart
Tested-by: Laurent Pinchart
> ---
> V2: Replace ifdef with __maybe_unused
> Simplify return value handling in resume
> V3: Drop useless regcache_sync() call from suspend callback
> (thanks Laurent for testing with working DU)
> ---
> drivers/clk/clk-versaclock5.c | 25 +
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c
> index decffb3826ec..5b393e711e94 100644
> --- a/drivers/clk/clk-versaclock5.c
> +++ b/drivers/clk/clk-versaclock5.c
> @@ -906,6 +906,28 @@ static int vc5_remove(struct i2c_client *client)
> return 0;
> }
>
> +static int __maybe_unused vc5_suspend(struct device *dev)
> +{
> + struct vc5_driver_data *vc5 = dev_get_drvdata(dev);
> +
> + regcache_cache_only(vc5->regmap, true);
> + regcache_mark_dirty(vc5->regmap);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused vc5_resume(struct device *dev)
> +{
> + struct vc5_driver_data *vc5 = dev_get_drvdata(dev);
> + int ret;
> +
> + regcache_cache_only(vc5->regmap, false);
> + ret = regcache_sync(vc5->regmap);
> + if (ret)
> + dev_err(dev, "Failed to restore register map: %d\n", ret);
> + return ret;
> +}
> +
> static const struct vc5_chip_info idt_5p49v5923_info = {
> .model = IDT_VC5_5P49V5923,
> .clk_fod_cnt = 2,
> @@ -961,9 +983,12 @@ static const struct of_device_id clk_vc5_of_match[] = {
> };
> MODULE_DEVICE_TABLE(of, clk_vc5_of_match);
>
> +static SIMPLE_DEV_PM_OPS(vc5_pm_ops, vc5_suspend, vc5_resume);
> +
> static struct i2c_driver vc5_driver = {
> .driver = {
> .name = "vc5",
> + .pm = &vc5_pm_ops,
> .of_match_table = clk_vc5_of_match,
> },
> .probe = vc5_probe,
--
Regards,
Laurent Pinchart
Re: [PATCH 2/2] pinctrl: sh-pfc: r8a77995: Fix MOD_SEL bit numbering
Hello Shimoda-san,
Thank you for the patch.
On Wednesday, 12 December 2018 12:19:35 EET Yoshihiro Shimoda wrote:
> From: Takeshi Kihara
>
> MOD_SEL register bit numbering was different from R-Car D3 SoC and
> R-Car H3/M3-[WN] SoCs.
>
> MOD_SEL 1-bit H3/M3-[WN] D3
> ===== =
> Set Value = H'0b'0 b'0
> Set Value = H'1b'1 b'1
>
> MOD_SEL 2-bits H3/M3-[WN] D3
> ===== =
> Set Value = H'0b'00b'00
> Set Value = H'1b'01b'10
> Set Value = H'2b'10b'01
> Set Value = H'3b'11b'11
>
> MOD_SEL 3-bits H3/M3-[WN] D3
> ===== =
> Set Value = H'0b'000 b'000
> Set Value = H'1b'001 b'100
> Set Value = H'2b'010 b'010
> Set Value = H'3b'011 b'110
> Set Value = H'4b'100 b'001
> Set Value = H'5b'101 b'101
> Set Value = H'6b'110 b'011
> Set Value = H'7b'111 b'111
>
> This patch replaces the #define name and value of MOD_SEL.
>
> Signed-off-by: Takeshi Kihara
> Fixes: 794a67117646 ("pinctrl: sh-pfc: Initial R8A77995 PFC support")
> [shimoda: split a patch per SoC and revise the commit log]
> Signed-off-by: Yoshihiro Shimoda
Per Geert's request I've tested this patch on the Draak board with pwm-
backlight, and it doesn't seem to make any difference.
> ---
> drivers/pinctrl/sh-pfc/pfc-r8a77995.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pinctrl/sh-pfc/pfc-r8a77995.c
> b/drivers/pinctrl/sh-pfc/pfc-r8a77995.c index e457539..e0db6f3 100644
> --- a/drivers/pinctrl/sh-pfc/pfc-r8a77995.c
> +++ b/drivers/pinctrl/sh-pfc/pfc-r8a77995.c
> @@ -388,10 +388,10 @@
> #define MOD_SEL0_27 FM(SEL_MSIOF3_0)FM(SEL_MSIOF3_1)
> #define MOD_SEL0_26 FM(SEL_HSCIF3_0)FM(SEL_HSCIF3_1)
> #define MOD_SEL0_25 FM(SEL_SCIF4_0) FM(SEL_SCIF4_1)
> -#define
> MOD_SEL0_24_23FM(SEL_PWM0_0) FM(SEL_PWM0_1)
> FM(SEL_PWM0_2)
FM(SEL_PWM0
> _3) -#define
> MOD_SEL0_22_21FM(SEL_PWM1_0) FM(SEL_PWM1_1)
> FM(SEL_PWM1_2)
FM(SEL_PWM1
> _3) -#define
> MOD_SEL0_20_19FM(SEL_PWM2_0) FM(SEL_PWM2_1)
> FM(SEL_PWM2_2)
FM(SEL_PWM2
> _3) -#define
> MOD_SEL0_18_17FM(SEL_PWM3_0) FM(SEL_PWM3_1)
> FM(SEL_PWM3_2)
FM(SEL_PWM3
> _3) +#define
> MOD_SEL0_24_23FM(SEL_PWM0_0) FM(SEL_PWM0_2)
> FM(SEL_PWM0_1)
FM(SEL_PWM0
> _3) +#define
> MOD_SEL0_22_21FM(SEL_PWM1_0) FM(SEL_PWM1_2)
> FM(SEL_PWM1_1)
FM(SEL_PWM1
> _3) +#define
> MOD_SEL0_20_19FM(SEL_PWM2_0) FM(SEL_PWM2_2)
> FM(SEL_PWM2_1)
FM(SEL_PWM2
> _3) +#define
> MOD_SEL0_18_17FM(SEL_PWM3_0) FM(SEL_PWM3_2)
> FM(SEL_PWM3_1)
FM(SEL_PWM3
> _3) #define MOD_SEL0_15 FM(SEL_IRQ_0_0) FM(SEL_IRQ_0_1)
> #define MOD_SEL0_14 FM(SEL_IRQ_1_0) FM(SEL_IRQ_1_1)
> #define MOD_SEL0_13 FM(SEL_IRQ_2_0) FM(SEL_IRQ_2_1)
--
Regards,
Laurent Pinchart
Re: [PATCH 5/5] media: adv748x: Implement link_setup callback
Hi Jacopo,
Thank you for the patch.
On Tuesday, 11 December 2018 17:16:13 EET Jacopo Mondi wrote:
> When the adv748x driver is informed about a link being created from HDMI or
> AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make
> sure to implement proper routing management at link setup time, to route
> the selected video stream to the desired TX output.
>
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/i2c/adv748x/adv748x-core.c | 63 -
> drivers/media/i2c/adv748x/adv748x.h | 1 +
> 2 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> b/drivers/media/i2c/adv748x/adv748x-core.c index f3aabbccdfb5..08dc0e89b053
> 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -335,9 +335,70 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
> /*
> * Media Operations
> */
> +static int adv748x_link_setup(struct media_entity *entity,
> + const struct media_pad *local,
> + const struct media_pad *remote, u32 flags)
> +{
> + struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity);
> + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> + struct adv748x_state *state = v4l2_get_subdevdata(sd);
> + struct adv748x_csi2 *tx;
> + struct media_link *link;
> + u8 io10;
> +
> + /*
> + * For each link setup from [HDMI|AFE] to TX we receive two
> + * notifications: "[HDMI|AFE]->TX" and "TX<-[HDMI|AFE]".
> + *
> + * Use the second notification form to make sure we're linking
> + * to a TX and find out from where, to set up routing properly.
> + */
Why don't you implement the link handler just for the TX entities then ?
> + if ((sd != &state->txa.sd && sd != &state->txb.sd) ||
> + !(flags & MEDIA_LNK_FL_ENABLED))
When disabling the link you should reset the ->source and ->tx pointers.
> + return 0;
> + tx = adv748x_sd_to_csi2(sd);
> +
> + /*
> + * Now that we're sure we're operating on one of the two TXs,
> + * make sure there are no enabled links ending there from
> + * either HDMI or AFE (this can only happens for TXA though).
> + */
> + if (is_txa(tx))
> + list_for_each_entry(link, &entity->links, list)
> + if (link->sink->entity == entity &&
> + link->flags & MEDIA_LNK_FL_ENABLED)
> + return -EINVAL;
You can simplify this by checking if tx->source == NULL (after resetting tx-
>source when disabling the link of course).
> + /* Change video stream routing, according to the newly created link. */
> + io10 = io_read(state, ADV748X_IO_10);
> + if (rsd == &state->afe.sd) {
> + state->afe.tx = tx;
> +
> + /*
> + * If AFE is routed to TXA, make sure TXB is off;
> + * If AFE goes to TXB, we need TXA powered on.
> + */
> + if (is_txa(tx)) {
> + io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
> + io10 &= ~ADV748X_IO_10_CSI1_EN;
> + } else {
> + io10 |= ADV748X_IO_10_CSI4_EN |
> + ADV748X_IO_10_CSI1_EN;
> + }
> + } else {
> + state->hdmi.tx = tx;
> + io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> + }
> + io_write(state, ADV748X_IO_10, io10);
Is it guaranteed that the chip will be powered on at this point ? How about
writing the register at stream on time instead ?
> + tx->rsd = rsd;
> +
> + return 0;
> +}
>
> static const struct media_entity_operations adv748x_media_ops = {
> - .link_validate = v4l2_subdev_link_validate,
> + .link_setup = adv748x_link_setup,
> + .link_validate = v4l2_subdev_link_validate,
> };
>
> /*
> -- diff --git a/drivers/media/i2c/adv748x/adv748x.h
> b/drivers/media/i2c/adv748x/adv748x.h index 0ee3b8d5c795..63a17c31c169
> 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -220,6 +220,7 @@ struct adv748x_state {
> #define ADV748X_IO_10_CSI4_ENBIT(7)
> #define ADV748X_IO_10_CSI1_ENBIT(6)
> #define ADV748X_IO_10_PIX_OUT_EN BIT(5)
> +#define ADV748X_IO_10_CSI4_IN_SEL_AFE0x08
>
> #define ADV748X_IO_CHIP_REV_ID_1 0xdf
> #define ADV748X_IO_CHIP_REV_ID_2 0xe0
--
Regards,
Laurent Pinchart
Re: [PATCH 4/5] media: adv748x: Store the TX sink in HDMI/AFE
Hi Jacopo,
Thank you for the patch.
On Tuesday, 11 December 2018 17:16:12 EET Jacopo Mondi wrote:
> Both the AFE and HDMI s_stream routines (adv748x_afe_s_stream() and
> adv748x_hdmi_s_stream()) have to enable the CSI-2 TX they are streaming
> video data to.
>
> With the introduction of dynamic routing between HDMI and AFE entities to
> TXA, the video stream sink needs to be set at run time, and not statically
> selected as the s_stream functions are currently doing.
>
> To fix this, store a reference to the active CSI-2 TX sink for both HDMI and
> AFE sources, and operate on it when starting/stopping the stream.
>
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/i2c/adv748x/adv748x-afe.c | 2 +-
> drivers/media/i2c/adv748x/adv748x-csi2.c | 19 ++-
> drivers/media/i2c/adv748x/adv748x-hdmi.c | 2 +-
> drivers/media/i2c/adv748x/adv748x.h | 4
> 4 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-afe.c
> b/drivers/media/i2c/adv748x/adv748x-afe.c index 71714634efb0..dbbb1e4d6363
> 100644
> --- a/drivers/media/i2c/adv748x/adv748x-afe.c
> +++ b/drivers/media/i2c/adv748x/adv748x-afe.c
> @@ -282,7 +282,7 @@ static int adv748x_afe_s_stream(struct v4l2_subdev *sd,
> int enable) goto unlock;
> }
>
> - ret = adv748x_tx_power(&state->txb, enable);
> + ret = adv748x_tx_power(afe->tx, enable);
> if (ret)
> goto unlock;
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c
> b/drivers/media/i2c/adv748x/adv748x-csi2.c index 307966f4c736..0d6344a51795
> 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -85,6 +85,9 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
> MEDIA_LNK_FL_ENABLED);
> if (ret)
> return ret;
> +
> + /* The default HDMI output is TXA. */
> + state->hdmi.tx = tx;
> }
>
> if (is_afe_enabled(state)) {
> @@ -95,11 +98,17 @@ static int adv748x_csi2_registered(struct v4l2_subdev
> *sd) if (ret)
> return ret;
> }
> - } else if (is_afe_enabled(state))
> - return adv748x_csi2_register_link(tx, sd->v4l2_dev,
> - &state->afe.sd,
> - ADV748X_AFE_SOURCE,
> - MEDIA_LNK_FL_ENABLED);
> + } else if (is_afe_enabled(state)) {
> + ret = adv748x_csi2_register_link(tx, sd->v4l2_dev,
> + &state->afe.sd,
> + ADV748X_AFE_SOURCE,
> + MEDIA_LNK_FL_ENABLED);
> + if (ret)
> + return ret;
> +
> + /* The default AFE output is TXB. */
> + state->afe.tx = tx;
> + }
>
> return 0;
> }
> diff --git a/drivers/media/i2c/adv748x/adv748x-hdmi.c
> b/drivers/media/i2c/adv748x/adv748x-hdmi.c index 35d027941482..c557f8fdf11a
> 100644
> --- a/drivers/media/i2c/adv748x/adv748x-hdmi.c
> +++ b/drivers/media/i2c/adv748x/adv748x-hdmi.c
> @@ -358,7 +358,7 @@ static int adv748x_hdmi_s_stream(struct v4l2_subdev *sd,
> int enable)
>
> mutex_lock(&state->mutex);
>
> - ret = adv748x_tx_power(&state->txa, enable);
> + ret = adv748x_tx_power(hdmi->tx, enable);
> if (ret)
> goto done;
>
> diff --git a/drivers/media/i2c/adv748x/adv748x.h
> b/drivers/media/i2c/adv748x/adv748x.h index 387002d6da65..0ee3b8d5c795
> 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -118,6 +118,8 @@ struct adv748x_hdmi {
> struct v4l2_dv_timings timings;
> struct v4l2_fract aspect_ratio;
>
> + struct adv748x_csi2 *tx;
> +
> struct {
> u8 edid[512];
> u32 present;
> @@ -148,6 +150,8 @@ struct adv748x_afe {
> struct v4l2_subdev sd;
> struct v4l2_mbus_framefmt format;
>
> + struct adv748x_csi2 *tx;
> +
> bool streaming;
> v4l2_std_id curr_norm;
> unsigned int input;
This may call for defining a common structure to store the common fields of
adv748x_hdmi and adv748x_afe. Out of scope for this patch, but please keep it
in mind.
Reviewed-by: Laurent Pinchart
--
Regards,
Laurent Pinchart
Re: [PATCH 3/5] media: adv748x: Store the source subdevice in TX
Hi Jacopo,
Thank you for the patch.
On Tuesday, 11 December 2018 17:16:11 EET Jacopo Mondi wrote:
> The power_up_tx() procedure needs to set a few registers conditionally to
> the selected video source, but it currently checks for the provided tx to
> be either TXA or TXB.
>
> With the introduction of dynamic routing between HDMI and AFE entities to
> TXA, checking which TX the function is operating on is not meaningful
> anymore.
>
> To fix this, store the subdevice of the source providing video data to the
> CSI-2 TX in the 'struct adv748x_csi2' representing the TX and check on it.
>
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/i2c/adv748x/adv748x-core.c | 2 +-
> drivers/media/i2c/adv748x/adv748x-csi2.c | 3 +++
> drivers/media/i2c/adv748x/adv748x.h | 1 +
> 3 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> b/drivers/media/i2c/adv748x/adv748x-core.c index 5495dc7891e8..f3aabbccdfb5
> 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -254,7 +254,7 @@ static int adv748x_power_up_tx(struct adv748x_csi2 *tx)
> adv748x_write_check(state, page, 0x00, 0xa0 | tx->num_lanes, &ret);
>
> /* ADI Required Write */
> - if (is_txa(tx)) {
> + if (tx->rsd == &state->hdmi.sd) {
> adv748x_write_check(state, page, 0xdb, 0x10, &ret);
> adv748x_write_check(state, page, 0xd6, 0x07, &ret);
> } else {
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c
> b/drivers/media/i2c/adv748x/adv748x-csi2.c index 4d1aefc2c8d0..307966f4c736
> 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -46,6 +46,9 @@ static int adv748x_csi2_register_link(struct adv748x_csi2
> *tx, return ret;
> }
>
> + if (flags & MEDIA_LNK_FL_ENABLED)
> + tx->rsd = src;
> +
> return media_create_pad_link(&src->entity, src_pad,
>&tx->sd.entity, ADV748X_CSI2_SINK,
>flags);
> diff --git a/drivers/media/i2c/adv748x/adv748x.h
> b/drivers/media/i2c/adv748x/adv748x.h index b482c7fe6957..387002d6da65
> 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -85,6 +85,7 @@ struct adv748x_csi2 {
> struct v4l2_ctrl_handler ctrl_hdl;
> struct v4l2_ctrl *pixel_rate;
> struct v4l2_subdev sd;
> + struct v4l2_subdev *rsd;
How about naming this source instead of rsd ? rsd is a bit cryptic.
With that change,
Reviewed-by: Laurent Pinchart
> };
>
> #define notifier_to_csi2(n) container_of(n, struct adv748x_csi2, notifier)
--
Regards,
Laurent Pinchart
Re: [PATCH 2/5] media: adv748x: csi2: Link AFE with TXA and TXB
&state->afe.sd,
> >> -ADV748X_AFE_SOURCE);
> >> +ADV748X_AFE_SOURCE,
> >> +MEDIA_LNK_FL_ENABLED);
> >
> > Won't this enable the AFE link for both TXA and TXB ?
> > Which one will win? Just the last one ? the first one ?
> > Does it error?
> >
> > (It might not be a problem ... I can't recall what the behaviour is)
>
> The AFE->TXA link is created as not enabled (see the 0 as last
> argument in the adv748x_csi2_register_link() call above here, in the
> 'is_txa(tx)' case
>
> >> +
> >
> > There are a lot of nested if's above, and I think we can simplify
> > greatly if we move the logic for the flags inside
> > adv748x_csi2_register_link(), and adjust the checks on is_xxx_enabled()
> >
> > What do you think about the following pseudo code?:
> >
> > int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
> > struct v4l2_device *v4l2_dev,
> > struct v4l2_subdev *src,
> > unsigned int src_pad,
> > bool enable)
> > {
> > int flags = 0;
> > int ret;
> >
> > if (!src->v4l2_dev) {
> > ret = v4l2_device_register_subdev(v4l2_dev, src)
> > if (ret) return ret;
> > }
> >
> > if (enable)
> > flags = MEDIA_LNK_FL_ENABLED;
> >
> >return media_create_pad_link(&src->entity, src_pad,
> > &tx->sd.entity, ADV748X_CSI2_SINK,
> > flags);
> > }
> >
> > int adv748x_csi2_registered(struct v4l2_subdev *sd)
> > {
> > int ret;
> >
I would add
/* The AFE can be routed to both TXA and TXB. */
> > if (is_afe_enabled(state) {
> > ret = adv748x_csi2_register_link(tx, sd->v4l2_dev, &state->afe.sd,
> >ADV748X_AFE_SOURCE, !is_txa(tx));
> > if (ret)
> > return ret;
> > }
> >
> > /* TX-B only supports AFE */
> > if (!is_txa(tx) || !(is_hdmi_enabled(state))
> > return 0;
> >
> > return adv748x_csi2_register_link(tx, sd->v4l2_dev, &state->hdmi.sd,
> > ADV748X_HDMI_SOURCE, true);
> > }
Looks better than my above proposal :-)
> > The above will for TXA:
> > register_link(..., AFE_SOURCE, enable = false );
> > register_link(..., HDMI_SOURCE, enable = true );
> >
> > then TXB:
> > register_link(..., AFE_SOURCE, enable = true );
> >
> > Does that meet our needs?
>
> Yes it does, and it looks better. Thanks!
>
> >>return 0;
> >> }
With Kieran's proposed change,
Reviewed-by: Laurent Pinchart
--
Regards,
Laurent Pinchart
Re: [PATCH 1/5] media: adv748x: Rework reset procedure
Hi Jacopo,
Thank you for the patch.
On Wednesday, 12 December 2018 12:13:44 EET Kieran Bingham wrote:
> On 12/12/2018 08:16, jacopo mondi wrote:
> > On Tue, Dec 11, 2018 at 11:52:03PM +, Kieran Bingham wrote:
> >> On 11/12/2018 15:16, Jacopo Mondi wrote:
> >>> Re-work the chip reset procedure to configure the CP (HDMI) and SD (AFE)
> >>> cores before resetting the MIPI CSI-2 TXs.
> >>>
> >>> Signed-off-by: Jacopo Mondi
> >>> ---
> >>>
> >>> drivers/media/i2c/adv748x/adv748x-core.c | 24 ++--
> >>> 1 file changed, 10 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> >>> b/drivers/media/i2c/adv748x/adv748x-core.c index
> >>> d94c63cb6a2e..5495dc7891e8 100644
> >>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> >>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> >>> @@ -353,9 +353,8 @@ static const struct adv748x_reg_value
> >>> adv748x_sw_reset[] = {>>>
> >>> {ADV748X_PAGE_EOR, 0xff, 0xff} /* End of register table */
> >>>
> >>> };
> >>>
> >>> -/* Supported Formats For Script Below */
> >>> -/* - 01-29 HDMI to MIPI TxA CSI 4-Lane - RGB888: */
> >>
> >> Is this information redundant ? (CSI-4Lane, RGB888 configuration?)
> >
> > The CSI-2 data lane configuration has been break out from this table
> > by Niklas' patches. I've tried also moving the format configuration
> > out of this, but I haven't sent that change. The HDMI video direction
> > is now handled at link setup time, so I guess the only relevant
> > information is about the RGB888 format configured on the CP backend.
> > I'll keep that.
>
> Thanks for the clarification.
Sounds good to me. With this change,
Reviewed-by: Laurent Pinchart
> >>> -static const struct adv748x_reg_value adv748x_init_txa_4lane[] = {
> >>> +/* Initialize CP Core. */
> >>> +static const struct adv748x_reg_value adv748x_init_hdmi[] = {
> >>
> >> While we're here - is there much scope - or value in changing these
> >> tables to functions with parameters using Niklas' adv748x_write_check() ?
> >>
> >> The suggestion only has value if there are parameters that we would need
> >> to configure. So it might be reasonable to leave these tables.
> >
> > Right now I don't see much value in that. I would prefer breaking out
> > the format configuration from this static tables, but that's for
> > later.
>
> Perfect - I agree - doesn't need to happen in this patch.
>
> If the format configuration can be broken out from the table later then
> that's great news.
I think it will make sense to do so, yes.
> >> A general Ack on renaming to the function instead of the
> >> TX/configuration though - as that makes the purpose clearer.
> >>
> >>> /* Disable chip powerdown & Enable HDMI Rx block */
> >>> {ADV748X_PAGE_IO, 0x00, 0x40},
> >>>
> >>> @@ -399,10 +398,8 @@ static const struct adv748x_reg_value
> >>> adv748x_init_txa_4lane[] = {>>>
> >>> {ADV748X_PAGE_EOR, 0xff, 0xff} /* End of register table */
> >>>
> >>> };
> >>>
> >>> -/* 02-01 Analog CVBS to MIPI TX-B CSI 1-Lane - */
> >>> -/* Autodetect CVBS Single Ended In Ain 1 - MIPI Out */
> >>> -static const struct adv748x_reg_value adv748x_init_txb_1lane[] = {
> >>> -
> >>
> >> Same comments as above really :)
> >
> > I'll see what I can keep.
> >
> > Thanks
> >
> > j
> >
> >>> +/* Initialize AFE core. */
> >>> +static const struct adv748x_reg_value adv748x_init_afe[] = {
> >>>
> >>> {ADV748X_PAGE_IO, 0x00, 0x30}, /* Disable chip powerdown Rx */
> >>> {ADV748X_PAGE_IO, 0xf2, 0x01}, /* Enable I2C Read Auto-Increment */
> >>>
> >>> @@ -445,19 +442,18 @@ static int adv748x_reset(struct adv748x_state
> >>> *state)
> >>>
> >>> if (ret < 0)
> >>>
> >>> return ret;
> >>>
> >>> - /* Init and power down TXA */
> >>> - ret = adv748x_write_regs(state, adv748x_init_txa_4lane);
> >>> + /* Initialize CP and AFE cores. */
> >>> + ret = adv748x_write_regs(state, adv748x_init_hdmi);
> >>>
> >>> if (ret)
> >>>
> >>> return ret;
> >>>
> >>> - adv748x_tx_power(&state->txa, 1);
> >>> - adv748x_tx_power(&state->txa, 0);
> >>> -
> >>> - /* Init and power down TXB */
> >>> - ret = adv748x_write_regs(state, adv748x_init_txb_1lane);
> >>> + ret = adv748x_write_regs(state, adv748x_init_afe);
> >>>
> >>> if (ret)
> >>>
> >>> return ret;
> >>>
> >>> + /* Reset TXA and TXB */
> >>> + adv748x_tx_power(&state->txa, 1);
> >>> + adv748x_tx_power(&state->txa, 0);
> >>>
> >>> adv748x_tx_power(&state->txb, 1);
> >>> adv748x_tx_power(&state->txb, 0);
> >>>
> >>> --
> >>> 2.7.4
> >>
> >> --
> >> Regards
> >> --
> >> Kieran
--
Regards,
Laurent Pinchart
Re: [PATCH V2] clk: vc5: Add suspend/resume support
Hi Marek,
On Thursday, 13 December 2018 04:09:19 EET Marek Vasut wrote:
> On 12/12/2018 09:43 AM, Laurent Pinchart wrote:
> > On Wednesday, 12 December 2018 03:41:30 EET Marek Vasut wrote:
> >> Add simple suspend/resume handlers to the driver to restore the chip
> >> configuration after resume. It is possible that the chip was configured
> >> with non-default values before suspend-resume cycle and that the chip
> >> is powered down during this cycle, so the configuration could get lost.
> >>
> >> Signed-off-by: Marek Vasut
> >> Cc: Alexey Firago
> >> Cc: Laurent Pinchart
> >> Cc: Michael Turquette
> >> Cc: Stephen Boyd
> >> Cc: linux-renesas-soc@vger.kernel.org
> >> ---
> >> V2: Replace ifdef with __maybe_unused
> >>
> >> Simplify return value handling in resume
> >>
> >> ---
> >>
> >> drivers/clk/clk-versaclock5.c | 31 +++
> >> 1 file changed, 31 insertions(+)
> >>
> >> diff --git a/drivers/clk/clk-versaclock5.c
> >> b/drivers/clk/clk-versaclock5.c
> >> index decffb3826ec..b66586a3abb7 100644
> >> --- a/drivers/clk/clk-versaclock5.c
> >> +++ b/drivers/clk/clk-versaclock5.c
> >> @@ -906,6 +906,34 @@ static int vc5_remove(struct i2c_client *client)
> >>return 0;
> >> }
> >>
> >> +static int __maybe_unused vc5_suspend(struct device *dev)
> >> +{
> >> + struct vc5_driver_data *vc5 = dev_get_drvdata(dev);
> >> + int ret;
> >> +
> >> + ret = regcache_sync(vc5->regmap);
> >
> > Didn't you say the sync here was unneeded and would be dropped ?
> >
> >> + if (ret != 0) {
> >> + dev_err(dev, "Failed to save register map: %d\n", ret);
> >> + return ret;
> >> + }
>
> If you have a setup with working DU and VGA on something close to next
> (it's broken in next), can you try dropping this hunk (basically do
> ret = 0;//regcache_sync(vc5->regmap); ) and see if the regcache stays
> consistent ? It should. If so, I'll drop this in V3.
>
> >> + regcache_cache_only(vc5->regmap, true);
> >> + regcache_mark_dirty(vc5->regmap);
>
> [...]
Here's my test procedure:
- Boot the board
- Dump the VC5 state (into vc5-next+*-1-boot.log)
- Enable the VGA output with kmstest -c VGA-1
- Dump the VC5 state (into vc5-next+*-2-display-on.log)
- Suspend and resume
- Dump the VC5 state (into vc5-next+*-3-post-suspend.log)
- Stop kmstest
- Dump the VC5 state (into vc5-next+*-4-display-off.log)
To dump the VC5 state, I use
-
#!/bin/sh
echo " i2cdump "
i2cdump -f -y 4 0x6a
for f in /sys/kernel/debug/regmap/4-006a/* ; do
echo " $f "
cat $f
done
-
The base kernel version is v4.20-rc6 + the fixes branch from linux media. I've
tested the following three configurations, in order:
next+0 - Base
next+1 - Base + this patch
next+2 - Base + this patch + removal of regcache_sync() from vc5_suspend()
With base, the VGA output would remain off after resume, and running kmstest
again wouldn't help. With the other two configurations the problem is fixed
and the VGA output is functional.
Furthermore, there are no differences in the VC5 dumps between next+1 and
next+2, neither are there between the boot and display-on dumps between any of
the three configurations.
I've attached the logs to this e-mail.
--
Regards,
Laurent Pinchart
i2cdump
0 1 2 3 4 5 6 7 8 9 a b c d e f0123456789abcdef
00: 61 ff 00 00 00 00 00 00 00 ff 01 c0 00 b6 b4 92a.??.???
10: 8a 2a 81 80 00 03 84 03 60 00 00 00 1f fd f0 80?*??.???`...
20: 00 81 00 5d 17 44 00 00 00 00 04 00 00 00 90 00.?.]?D?...?.
30: 00 81 02 00 00 00 00 00 00 00 04 00 00 00 c0 00.??...?...?.
40: 00 00 00 00 00 00 00 00 00 00 04 00 00 00 00 00..?.
50: 00 00 00 00 00 00 00 00 00 00 04 00 00 00 00 00..?.
60: 23 01 23 01 23 00 23 00 fc e4 00 00 00 00 00 30#?#?#.#.??.0
70: 00 60 f0 00 4e 00 63 00 00 10 02 08 6e 05 14 02.`?.N.c..???n???
80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
90: 00 00 00 00 00 00 00 00 00 56 00 00 00 90 94 b4.V...???
a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
d0: ff ff ff ff ff ff ff f
Re: [PATCH V2] clk: vc5: Add suspend/resume support
Hi Marex,
Thank you for the patch.
On Wednesday, 12 December 2018 03:41:30 EET Marek Vasut wrote:
> Add simple suspend/resume handlers to the driver to restore the chip
> configuration after resume. It is possible that the chip was configured
> with non-default values before suspend-resume cycle and that the chip
> is powered down during this cycle, so the configuration could get lost.
>
> Signed-off-by: Marek Vasut
> Cc: Alexey Firago
> Cc: Laurent Pinchart
> Cc: Michael Turquette
> Cc: Stephen Boyd
> Cc: linux-renesas-soc@vger.kernel.org
> ---
> V2: Replace ifdef with __maybe_unused
> Simplify return value handling in resume
> ---
> drivers/clk/clk-versaclock5.c | 31 +++
> 1 file changed, 31 insertions(+)
>
> diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c
> index decffb3826ec..b66586a3abb7 100644
> --- a/drivers/clk/clk-versaclock5.c
> +++ b/drivers/clk/clk-versaclock5.c
> @@ -906,6 +906,34 @@ static int vc5_remove(struct i2c_client *client)
> return 0;
> }
>
> +static int __maybe_unused vc5_suspend(struct device *dev)
> +{
> + struct vc5_driver_data *vc5 = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = regcache_sync(vc5->regmap);
Didn't you say the sync here was unneeded and would be dropped ?
> + if (ret != 0) {
> + dev_err(dev, "Failed to save register map: %d\n", ret);
> + return ret;
> + }
> + regcache_cache_only(vc5->regmap, true);
> + regcache_mark_dirty(vc5->regmap);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused vc5_resume(struct device *dev)
> +{
> + struct vc5_driver_data *vc5 = dev_get_drvdata(dev);
> + int ret;
> +
> + regcache_cache_only(vc5->regmap, false);
> + ret = regcache_sync(vc5->regmap);
> + if (ret)
> + dev_err(dev, "Failed to restore register map: %d\n", ret);
> + return ret;
> +}
> +
> static const struct vc5_chip_info idt_5p49v5923_info = {
> .model = IDT_VC5_5P49V5923,
> .clk_fod_cnt = 2,
> @@ -961,9 +989,12 @@ static const struct of_device_id clk_vc5_of_match[] = {
> };
> MODULE_DEVICE_TABLE(of, clk_vc5_of_match);
>
> +static SIMPLE_DEV_PM_OPS(vc5_pm_ops, vc5_suspend, vc5_resume);
> +
> static struct i2c_driver vc5_driver = {
> .driver = {
> .name = "vc5",
> + .pm = &vc5_pm_ops,
> .of_match_table = clk_vc5_of_match,
> },
> .probe = vc5_probe,
--
Regards,
Laurent Pinchart
Re: [PATCH] media: rcar-csi2: Fix PHTW table values for E3/V3M
Hi Niklas,
On Tuesday, 11 December 2018 04:01:15 EET Niklas Söderlund wrote:
> On 2018-12-10 22:16:52 +0200, Laurent Pinchart wrote:
> > On Monday, 10 December 2018 16:53:55 EET Jacopo Mondi wrote:
> >> The PHTW selection algorithm implemented in rcsi2_phtw_write_mbps()
> >> checks for lower bound of the interval used to match the desired
> >> bandwidth. Use that in place of the currently used upport bound.
> >
> > The rcsi2_phtw_write_mbps() function performs the following (error
> > handling removed):
> >
> > const struct rcsi2_mbps_reg *value;
> >
> > for (value = values; value->mbps; value++)
> > if (value->mbps >= mbps)
> > break;
> >
> > return rcsi2_phtw_write(priv, value->reg, code);
> >
> > With this patch, an mbps value of 85 will match the second entry in the
> > phtw_mbps_v3m_e3 table:
> >
> > [0] { .mbps = 80, .reg = 0x00 },
> > [1] { .mbps = 90, .reg = 0x20 },
> > ...
> >
> > The datasheet however documents the range 80-89 to map to 0x00.
> >
> > What am I missing ?
>
> I'm afraid you are missing a issue with the original implementation of
> the rcar-csi2 driver (my fault). The issue you point out is a problem
> with the current freq selection logic not the tables themself which
> needs to be corrected.
>
> This patch aligns the table with the other tables in the driver and is
> sound. A patch (Jacopo care to submit it?) is needed to resolve the
> faulty logic in the driver. It should select the range according to
> Laurents findings and not the range above it as the current code does.
I wonder whether we should instead modify the tables, to avoid making the
selection logic more complicated and less efficient CPU-wise.
Speaking of which, the tables are interestingly specified in three different
ways in the datasheet:
- The TESTDIN_DATA table specifies non-overlapping ranges
e.g. [80, 89]: 0x00, [90, 99]: 0x20, [100, 109]: 0x40, ...
In this regard this patch is an improvement (that is if the faulty selection
logic gets fixed), as otherwise a frequency of 109.5 would be classified in
the [110, 129] range, while I think it is meant to be in the [100, 109] range.
Another option would be to set the table mbps value to the high bound of the
range:
{ .mbps = 90, .reg = 0x00 },
{ .mbps = 100, .reg = 0x20 },
{ .mbps = 110, .reg = 0x40 },
...
and use strict lower comparison logic:
const struct rcsi2_mbps_reg *value;
for (value = values; value->mbps; value++)
if (mbps < value->mbps)
break
- The PHTW table specifies individual bit rates
e.g. 80: 0x86, 90: 0x86, 100: 0x87, ..
I'm not sure how to interpret this. If I had to guess, I would say it means
[80, 90[: 0x86, [90, 100[: 0x86, [100, 110[: 0x87, ..
We could thus use the same logic than for the TESTDIN_DATA table (and while at
it merge adjacent ranges that share the same PHTW value).
- The HSFREQRANGE tables specify overlapping ranges
e.g. [80, 131.25]: 0x20, [80.75, 141.75]: 0x30, [90.25, 152.25]: 0x01, ...
This has to be converted to non-overlapping ranges. I would advice centering
each non-overlapping range to the center of the corresponding overlapping
range. Today we instead have non-overlapping ranges in the driver whose bounds
are set to the center of the overlapping ranges, and I don't think this is
right.
And we could then use the same logic as above here too.
The downside is that the tables would need to be carefully reviewed as they
would derive from the values in the datasheet instead of computing them
blindly, and the upside would be simpler code. If we want to instead copy the
tables blindly for ease of review, then I think we'll need more complex
selection logic, with different logics for the different tables.
> >> Fixes: 10c08812fe60 ("media: rcar: rcar-csi2: Update V3M/E3 PHTW
> >> tables")
> >> Signed-off-by: Jacopo Mondi
> >> ---
> >>
> >> drivers/media/platform/rcar-vin/rcar-csi2.c | 62 +++
> >> 1 file changed, 31 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> >> b/drivers/media/platform/rcar-vin/rcar-csi2.c index
> >> 80ad906d1136..7e9cb8bcfe70 100644
> >> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> >> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> >> @@ -152,37 +152,37 @@ static const struct rcsi2_mbps_reg
> >> phtw_mbps_h3_v3h_m3n[] = { };
> >>
> >> static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
> >> -
Re: [PATCH] media: vsp1: Fix trivial documentation
Hi Kieran,
Thank you for the patch.
On Friday, 7 December 2018 18:31:34 EET Kieran Bingham wrote:
> In the partition sizing the term 'prevents' is inappropriately
> pluralized. Simplify to 'prevent'.
>
> Signed-off-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
and applied to my tree.
> ---
> drivers/media/platform/vsp1/vsp1_video.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
> b/drivers/media/platform/vsp1/vsp1_video.c index 771dfe1f7c20..7ceaf3222145
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -223,7 +223,7 @@ static void vsp1_video_calculate_partition(struct
> vsp1_pipeline *pipe, * If the modulus is less than half of the partition
> size,
>* the penultimate partition is reduced to half, which is added
>* to the final partition: |1234|1234|1234|12|341|
> - * to prevents this: |1234|1234|1234|1234|1|.
> + * to prevent this:|1234|1234|1234|1234|1|.
> */
> if (modulus) {
> /*
--
Regards,
Laurent Pinchart
Re: [PATCH] media: rcar-csi2: Fix PHTW table values for E3/V3M
Hi Jacopo,
Thank you for the patch.
On Monday, 10 December 2018 16:53:55 EET Jacopo Mondi wrote:
> The PHTW selection algorithm implemented in rcsi2_phtw_write_mbps() checks
> for lower bound of the interval used to match the desired bandwidth. Use
> that in place of the currently used upport bound.
The rcsi2_phtw_write_mbps() function performs the following (error handling
removed):
const struct rcsi2_mbps_reg *value;
for (value = values; value->mbps; value++)
if (value->mbps >= mbps)
break;
return rcsi2_phtw_write(priv, value->reg, code);
With this patch, an mbps value of 85 will match the second entry in the
phtw_mbps_v3m_e3 table:
[0] { .mbps = 80, .reg = 0x00 },
[1] { .mbps = 90, .reg = 0x20 },
...
The datasheet however documents the range 80-89 to map to 0x00.
What am I missing ?
> Fixes: 10c08812fe60 ("media: rcar: rcar-csi2: Update V3M/E3 PHTW tables")
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/platform/rcar-vin/rcar-csi2.c | 62 -
> 1 file changed, 31 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> b/drivers/media/platform/rcar-vin/rcar-csi2.c index
> 80ad906d1136..7e9cb8bcfe70 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -152,37 +152,37 @@ static const struct rcsi2_mbps_reg
> phtw_mbps_h3_v3h_m3n[] = { };
>
> static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
> - { .mbps = 89, .reg = 0x00 },
> - { .mbps = 99, .reg = 0x20 },
> - { .mbps = 109, .reg = 0x40 },
> - { .mbps = 129, .reg = 0x02 },
> - { .mbps = 139, .reg = 0x22 },
> - { .mbps = 149, .reg = 0x42 },
> - { .mbps = 169, .reg = 0x04 },
> - { .mbps = 179, .reg = 0x24 },
> - { .mbps = 199, .reg = 0x44 },
> - { .mbps = 219, .reg = 0x06 },
> - { .mbps = 239, .reg = 0x26 },
> - { .mbps = 249, .reg = 0x46 },
> - { .mbps = 269, .reg = 0x08 },
> - { .mbps = 299, .reg = 0x28 },
> - { .mbps = 329, .reg = 0x0a },
> - { .mbps = 359, .reg = 0x2a },
> - { .mbps = 399, .reg = 0x4a },
> - { .mbps = 449, .reg = 0x0c },
> - { .mbps = 499, .reg = 0x2c },
> - { .mbps = 549, .reg = 0x0e },
> - { .mbps = 599, .reg = 0x2e },
> - { .mbps = 649, .reg = 0x10 },
> - { .mbps = 699, .reg = 0x30 },
> - { .mbps = 749, .reg = 0x12 },
> - { .mbps = 799, .reg = 0x32 },
> - { .mbps = 849, .reg = 0x52 },
> - { .mbps = 899, .reg = 0x72 },
> - { .mbps = 949, .reg = 0x14 },
> - { .mbps = 999, .reg = 0x34 },
> - { .mbps = 1049, .reg = 0x54 },
> - { .mbps = 1099, .reg = 0x74 },
> + { .mbps = 80, .reg = 0x00 },
> + { .mbps = 90, .reg = 0x20 },
> + { .mbps = 100, .reg = 0x40 },
> + { .mbps = 110, .reg = 0x02 },
> + { .mbps = 130, .reg = 0x22 },
> + { .mbps = 140, .reg = 0x42 },
> + { .mbps = 150, .reg = 0x04 },
> + { .mbps = 170, .reg = 0x24 },
> + { .mbps = 180, .reg = 0x44 },
> + { .mbps = 200, .reg = 0x06 },
> + { .mbps = 220, .reg = 0x26 },
> + { .mbps = 240, .reg = 0x46 },
> + { .mbps = 250, .reg = 0x08 },
> + { .mbps = 270, .reg = 0x28 },
> + { .mbps = 300, .reg = 0x0a },
> + { .mbps = 330, .reg = 0x2a },
> + { .mbps = 360, .reg = 0x4a },
> + { .mbps = 400, .reg = 0x0c },
> + { .mbps = 450, .reg = 0x2c },
> + { .mbps = 500, .reg = 0x0e },
> + { .mbps = 550, .reg = 0x2e },
> + { .mbps = 600, .reg = 0x10 },
> + { .mbps = 650, .reg = 0x30 },
> + { .mbps = 700, .reg = 0x12 },
> + { .mbps = 750, .reg = 0x32 },
> + { .mbps = 800, .reg = 0x52 },
> + { .mbps = 850, .reg = 0x72 },
> + { .mbps = 900, .reg = 0x14 },
> + { .mbps = 950, .reg = 0x34 },
> + { .mbps = 1000, .reg = 0x54 },
> + { .mbps = 1050, .reg = 0x74 },
> { .mbps = 1125, .reg = 0x16 },
> { /* sentinel */ },
> };
--
Regards,
Laurent Pinchart
Re: [PATCH 4/5] arm64: dts: renesas: r8a77995: draak: Add backlight
Hi Geert, On Monday, 10 December 2018 14:30:22 EET Geert Uytterhoeven wrote: > On Tue, Dec 4, 2018 at 6:36 PM Geert Uytterhoeven wrote: > > On Sun, Nov 25, 2018 at 3:40 PM Laurent Pinchart wrote: > >> Add the backlight device for the LVDS1 output, in preparation for panel > >> support. > >> > >> Signed-off-by: Laurent Pinchart > >> > > > > Reviewed-by: Geert Uytterhoeven > > Oops, seems I missed the backlight node should be moved up, to preserve > sort order. I assumed that aliases and chosen should be kept at the top of the file. Maybe we don't want to keep the tradition :-) -- Regards, Laurent Pinchart
Re: [PATCH v2] Add support to reset device controls
> int do_list_controls = 0, do_get_control = 0, do_set_control = 0;
> + int do_reset_controls = 0;
> int do_sleep_forever = 0, do_requeue_last = 0;
> int do_rt = 0, do_log_status = 0;
> int no_query = 0, do_queue_late = 0;
> @@ -2476,6 +2522,9 @@ int main(int argc, char *argv[])
> case OPT_QUEUE_LATE:
> do_queue_late = 1;
> break;
> + case OPT_RESET_CONTROLS:
> + do_reset_controls = 1;
> + break;
> case OPT_REQUEUE_LAST:
> do_requeue_last = 1;
> break;
> @@ -2560,7 +2609,10 @@ int main(int argc, char *argv[])
> video_set_control(&dev, ctrl_name, ctrl_value);
>
> if (do_list_controls)
> - video_list_controls(&dev);
> + video_list_controls(&dev, false);
> +
> + if (do_reset_controls)
> + video_list_controls(&dev, true);
>
> if (do_enum_formats) {
> printf("- Available formats:\n");
--
Regards,
Laurent Pinchart
[PATCH] arm64: dts: renesas: draak: Fix CVBS input
A typo in the adv7180 DT node prevents successful probing of the VIN.
Fix it.
Fixes: 6a0942c20f5c ("arm64: dts: renesas: draak: Describe CVBS input")
Signed-off-by: Laurent Pinchart
---
arch/arm64/boot/dts/renesas/r8a77995-draak.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
index 2405eaad0296..0f2523296b8a 100644
--- a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
@@ -195,7 +195,7 @@
compatible = "adi,adv7180cp";
reg = <0x20>;
- port {
+ ports {
#address-cells = <1>;
#size-cells = <0>;
--
Regards,
Laurent Pinchart
Re: [PATCH 0/5] pwm: rcar: Add support "atomic" API and workaround
Hi Shimoda-san, Thank you for the patches. On Friday, 7 December 2018 10:29:28 EET Yoshihiro Shimoda wrote: > This patch adds support for PWM "atomic" API. > > This patch also adds a workaround to output "pseudo" low level. > Otherwise, the PWM backlight driver doesn't work correctly, especially > it cannot output maximum brightness actually. > > Yoshihiro Shimoda (5): > pwm: rcar: add rcar_pwm_calc_counter() to calculate PWMCNT value only > pwm: rcar: Add support "atomic" API > pwm: rcar: Use "atomic" API on rcar_pwm_resume() > pwm: rcar: remove legacy APIs > pwm: rcar: add workaround to output "pseudo" low level > > drivers/pwm/pwm-rcar.c | 108 ++---- > 1 file changed, 62 insertions(+), 46 deletions(-) For the whole series, Tested-by: Laurent Pinchart with backlight on the Draak board. I do however agree with Uwe's comments. -- Regards, Laurent Pinchart
Re: pwm: rcar: improve calculation of divider
Hi Uwe,
On Friday, 7 December 2018 23:49:32 EET Uwe Kleine-König wrote:
> Hello,
>
> while looking at the driver I noticed another patch opportunity: In
> rcar_pwm_get_clock_division() there is a loop:
>
> for (div = 0; div <= RCAR_PWM_MAX_DIVISION; div++) {
> max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE *
> (1 << div);
> do_div(max, clk_rate);
> if (period_ns <= max)
> break;
> }
>
> The value of div should be calculatable without a loop. Something like:
>
>divider = NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE;
>tmp = (unsigned long long)period_ns * clk_rate + (divider - 1);
>do_div(tmp, divider);
>div = ilog2(tmp - 1) + 1;
>
> You might want to check if my maths are right, I didn't test.
I've noticed the same, and wrote the following patch last week, also untested.
I was planning to give it a try before sending it out, but as you've noticed
the same issue, here's the code if anyone wants to give it a try before I can.
Our calculations are similar, the main difference is the last line, and I
think yours read better.
From 22f7149916f590d3dbcc673dacc738441c741900 Mon Sep 17 00:00:00 2001
From: Laurent Pinchart
Date: Sun, 25 Nov 2018 16:02:39 +0200
Subject: [PATCH] pwm: rcar: Optimize rcar_pwm_get_clock_division()
Get rid of the loop over all possible divisor values by computing the
divisor directly.
Signed-off-by: Laurent Pinchart
---
drivers/pwm/pwm-rcar.c | 26 --
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
index a41812fc6f95..e6d73b94d5cf 100644
--- a/drivers/pwm/pwm-rcar.c
+++ b/drivers/pwm/pwm-rcar.c
@@ -68,21 +68,27 @@ static void rcar_pwm_update(struct rcar_pwm_chip *rp, u32
mask, u32 data,
static int rcar_pwm_get_clock_division(struct rcar_pwm_chip *rp, int period_ns)
{
unsigned long clk_rate = clk_get_rate(rp->clk);
- unsigned long long max; /* max cycle / nanoseconds */
- unsigned int div;
+ u64 max_period_ns;
+ u32 div;
if (clk_rate == 0)
return -EINVAL;
- for (div = 0; div <= RCAR_PWM_MAX_DIVISION; div++) {
- max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE *
- (1 << div);
- do_div(max, clk_rate);
- if (period_ns <= max)
- break;
- }
+ /*
+* The maximum achievable period is 2^24 * 1023 cycles of the internal
+* bus clock.
+*/
+ max_period_ns = (1ULL << RCAR_PWM_MAX_DIVISION) * RCAR_PWM_MAX_CYCLE
+ * NSEC_PER_SEC;
+ do_div(max_period_ns, clk_rate);
+
+ if (period_ns > max_period_ns)
+ return -ERANGE;
- return (div <= RCAR_PWM_MAX_DIVISION) ? div : -ERANGE;
+ /* Calculate the divisor and round it up to the next power of two. */
+ div = DIV64_U64_ROUND_UP((u64)period_ns * clk_rate,
+(u64)RCAR_PWM_MAX_CYCLE * NSEC_PER_SEC);
+ return fls(2 * div - 1) - 1;
}
static void rcar_pwm_set_clock_control(struct rcar_pwm_chip *rp,
--
Regards,
Laurent Pinchart
[PATCH v1.1 3/5] drm: rcar-du: Disable unused DPAD outputs
DU channels are routed to DPAD outputs in an SoC-dependent way. The
routing can be fixed (e.g. DU3 to DPAD0 on H3) or configurable (e.g. DU0
or DU1 to DPAD0 on D3/E3). The hardware offers no option to disconnect
DPAD outputs, which are thus always driven by a DU channel.
On SoCs that have less DU channels than DU outputs, such as D3 and E3,
the DPAD output is always driven when all channels are in use by other
outputs (such as the internal LVDS and HDMI encoders). This creates an
unwanted clone on the DPAD output.
However, the parallel output of the DU channels routed to DPAD can be
set to fixed levels in the DU channels themselves through the DOFLR
group register. Use this to turn the DPAD on or off by driving fixed
signals at the output of any DU channel not routed to a DPAD output.
This doesn't affect the DU output signals going to other outputs.
Signed-off-by: Laurent Pinchart
Reviewed-by: Kieran Bingham
---
Changes since v1:
- Renamed rcar_du_group_set_output_levels() to
rcar_du_group_set_dpad_levels()
- Clarify that fixed DPAD outputs are low-level
---
drivers/gpu/drm/rcar-du/rcar_du_group.c | 43 +
1 file changed, 43 insertions(+)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c
b/drivers/gpu/drm/rcar-du/rcar_du_group.c
index 7e440f61977f..9eee47969e77 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
@@ -287,6 +287,47 @@ int rcar_du_set_dpad0_vsp1_routing(struct rcar_du_device
*rcdu)
return 0;
}
+static void rcar_du_group_set_dpad_levels(struct rcar_du_group *rgrp)
+{
+ static const u32 doflr_values[2] = {
+ DOFLR_HSYCFL0 | DOFLR_VSYCFL0 | DOFLR_ODDFL0 |
+ DOFLR_DISPFL0 | DOFLR_CDEFL0 | DOFLR_RGBFL0,
+ DOFLR_HSYCFL1 | DOFLR_VSYCFL1 | DOFLR_ODDFL1 |
+ DOFLR_DISPFL1 | DOFLR_CDEFL1 | DOFLR_RGBFL1,
+ };
+ static const u32 dpad_mask = BIT(RCAR_DU_OUTPUT_DPAD1)
+ | BIT(RCAR_DU_OUTPUT_DPAD0);
+ struct rcar_du_device *rcdu = rgrp->dev;
+ u32 doflr = DOFLR_CODE;
+ unsigned int i;
+
+ if (rcdu->info->gen < 2)
+ return;
+
+ /*
+* The DPAD outputs can't be controlled directly. However, the parallel
+* output of the DU channels routed to DPAD can be set to fixed levels
+* through the DOFLR group register. Use this to turn the DPAD on or off
+* by driving fixed low-level signals at the output of any DU channel
+* not routed to a DPAD output. This doesn't affect the DU output
+* signals going to other outputs, such as the internal LVDS and HDMI
+* encoders.
+*/
+
+ for (i = 0; i < rgrp->num_crtcs; ++i) {
+ struct rcar_du_crtc_state *rstate;
+ struct rcar_du_crtc *rcrtc;
+
+ rcrtc = &rcdu->crtcs[rgrp->index * 2 + i];
+ rstate = to_rcar_crtc_state(rcrtc->crtc.state);
+
+ if (!(rstate->outputs & dpad_mask))
+ doflr |= doflr_values[i];
+ }
+
+ rcar_du_group_write(rgrp, DOFLR, doflr);
+}
+
int rcar_du_group_set_routing(struct rcar_du_group *rgrp)
{
struct rcar_du_device *rcdu = rgrp->dev;
@@ -306,5 +347,7 @@ int rcar_du_group_set_routing(struct rcar_du_group *rgrp)
rcar_du_group_write(rgrp, DORCR, dorcr);
+ rcar_du_group_set_dpad_levels(rgrp);
+
return rcar_du_set_dpad0_vsp1_routing(rgrp->dev);
}
--
Regards,
Laurent Pinchart
Re: [PATCH 2/5] drm: rcar-du: Move CRTC outputs bitmask to private CRTC state
Hi Kieran,
On Friday, 7 December 2018 14:34:58 EET Kieran Bingham wrote:
> On 25/11/2018 14:40, Laurent Pinchart wrote:
> > The rcar_du_crtc outputs field stores a bitmask of the outputs driven by
> > the CRTC. This changes based on the configuration requested by
> > userspace, and is used for the sole purpose of configuring the hardware.
> > The field thus belongs to the CRTC state. Move it to the
> > rcar_du_crtc_state structure.
> >
> > As a result the rcar_du_crtc_route_output() function loses most of its
> > purpose. In order to remove it, move dpad0_source calculation to
> > rcar_du_atomic_commit_tail(), until the field gets moved to a state
> > structure. In order to simplify the rcar_du_group_set_routing()
> > implementation, we also store the DPAD1 source in a new dpad1_source
> > field which will move to a state structure with dpad0_source.
> >
> > Signed-off-by: Laurent Pinchart
> >
>
> that was a fairly tough read - but aside from one comment near the
> bottom regarding initialising dpad0 which I'm sure you can handle
> correctly, and another comment which I think we could improve things
> outside of this patch:
>
> Reviewed-by: Kieran Bingham
>
> > ---
> >
> > drivers/gpu/drm/rcar-du/rcar_du_crtc.c| 42 +++
> > drivers/gpu/drm/rcar-du/rcar_du_crtc.h| 7 ++--
> > drivers/gpu/drm/rcar-du/rcar_du_drv.h | 1 +
> > drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 10 --
> > drivers/gpu/drm/rcar-du/rcar_du_group.c | 4 +--
> > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 22
> > 6 files changed, 47 insertions(+), 39 deletions(-)
>
> +8 ... It's a good job you bought -13 lines in the previous patch ;)
>
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 90dacab67be5..40b7f17159b0
> > 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > @@ -22,6 +22,7 @@
> >
> > #include "rcar_du_crtc.h"
> > #include "rcar_du_drv.h"
> > +#include "rcar_du_encoder.h"
> > #include "rcar_du_kms.h"
> > #include "rcar_du_plane.h"
> > #include "rcar_du_regs.h"
> > @@ -316,26 +317,6 @@ static void rcar_du_crtc_set_display_timing(struct
> > rcar_du_crtc *rcrtc)
> > rcar_du_crtc_write(rcrtc, DEWR, mode->hdisplay);
> > }
> >
> > -void rcar_du_crtc_route_output(struct drm_crtc *crtc,
> > - enum rcar_du_output output)
> > -{
> > - struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> > - struct rcar_du_device *rcdu = rcrtc->group->dev;
> > -
> > - /*
> > -* Store the route from the CRTC output to the DU output. The DU will
be
> > -* configured when starting the CRTC.
> > -*/
> > - rcrtc->outputs |= BIT(output);
> > -
> > - /*
> > -* Store RGB routing to DPAD0, the hardware will be configured when
> > -* starting the CRTC.
> > -*/
> > - if (output == RCAR_DU_OUTPUT_DPAD0)
> > - rcdu->dpad0_source = rcrtc->index;
> > -}
> > -
> > static unsigned int plane_zpos(struct rcar_du_plane *plane)
> > {
> > return plane->plane.state->normalized_zpos;
> > @@ -655,6 +636,24 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc
> > *rcrtc)
> > * CRTC Functions
> > */
> >
> > +static int rcar_du_crtc_atomic_check(struct drm_crtc *crtc,
> > +struct drm_crtc_state *state)
> > +{
> > + struct rcar_du_crtc_state *rstate = to_rcar_crtc_state(state);
> > + struct drm_encoder *encoder;
> > +
> > + /* Store the routes from the CRTC output to the DU outputs. */
> > + rstate->outputs = 0;
> > +
> > + drm_for_each_encoder_mask(encoder, crtc->dev, state->encoder_mask) {
> > + struct rcar_du_encoder *renc = to_rcar_encoder(encoder);
> > +
> > + rstate->outputs |= BIT(renc->output);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
> >struct drm_crtc_state *old_state)
> > {
> > @@ -678,8 +677,6 @@ static void rcar_du_crtc_atomic_disable(struct
> > drm_crtc *crtc,
> > crtc->state->event = NULL;
> > }
> > spin_unlock_irq(&crtc->dev->event_lock);
> > -
> > - rcrtc->outputs
Re: [PATCH 3/5] drm: rcar-du: Disable unused DPAD outputs
Hi Kieran,
On Friday, 7 December 2018 14:50:47 EET Kieran Bingham wrote:
> On 25/11/2018 14:40, Laurent Pinchart wrote:
> > DU channels are routed to DPAD outputs in an SoC-dependent way. The
> > routing can be fixed (e.g. DU3 to DPAD0 on H3) or configurable (e.g. DU0
> > or DU1 to DPAD0 on D3/E3). The hardware offers no option to disconnect
> > DPAD outputs, which are thus always driven by a DU channel.
> >
> > On SoCs that have less DU channels than DU outputs, such as D3 and E3,
> > the DPAD output is always driven when all channels are in used by other
>
> s/used/use/
>
> > outputs (such as the internal LVDS and HDMI encoders). This creates an
> > unwanted clone on the DPAD output.
> >
> > However, the parallel output of the DU channels routed to DPAD can be
> > set to fixed levels in the DU channels themselves through the DOFLR
> > group register. Use this to turn the DPAD on or off by driving fixed
> > signals at the output of any DU channel not routed to a DPAD output.
> > This doesn't affect the DU output signals going to other outputs.
> >
> > Signed-off-by: Laurent Pinchart
> >
>
> Only spelling and bikeshedding here - so:
>
> Reviewed-by: Kieran Bingham
>
> > ---
> >
> > drivers/gpu/drm/rcar-du/rcar_du_group.c | 42 +
> > 1 file changed, 42 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_group.c index
> > 7e440f61977f..5aaf41b7a2ca 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> > @@ -287,6 +287,46 @@ int rcar_du_set_dpad0_vsp1_routing(struct
> > rcar_du_device *rcdu)
> > return 0;
> > }
> >
> > +static void rcar_du_group_set_output_levels(struct rcar_du_group *rgrp)
> > +{
> > + static const u32 doflr_values[2] = {
> > + DOFLR_HSYCFL0 | DOFLR_VSYCFL0 | DOFLR_ODDFL0 |
> > + DOFLR_DISPFL0 | DOFLR_CDEFL0 | DOFLR_RGBFL0,
> > + DOFLR_HSYCFL1 | DOFLR_VSYCFL1 | DOFLR_ODDFL1 |
> > + DOFLR_DISPFL1 | DOFLR_CDEFL1 | DOFLR_RGBFL1,
> > + };
> > + static const u32 dpad_mask = BIT(RCAR_DU_OUTPUT_DPAD1)
> > + | BIT(RCAR_DU_OUTPUT_DPAD0);
> > + struct rcar_du_device *rcdu = rgrp->dev;
> > + u32 doflr = DOFLR_CODE;
> > + unsigned int i;
> > +
> > + if (rcdu->info->gen < 2)
> > + return;
> > +
> > + /*
> > +* The DPAD outputs can't be controlled directly. However, the parallel
> > +* output of the DU channels routed to DPAD can be set to fixed levels
> > +* through the DOFLR group register. Use this to turn the DPAD on or
off
> > +* by driving fixed signals at the output of any DU channel not routed
> > +* to a DPAD output. This doesn't affect the DU output signals going to
> > +* other outputs, such as the internal LVDS and HDMI encoders.
>
> Perhaps more out of interest - what /fixed/ levels do we output.
> High/Low/Hi-Z ?
It's low-level, I'll update the comment.
> > +*/
> > +
> > + for (i = 0; i < rgrp->num_crtcs; ++i) {
> > + struct rcar_du_crtc_state *rstate;
> > + struct rcar_du_crtc *rcrtc;
> > +
> > + rcrtc = &rcdu->crtcs[rgrp->index * 2 + i];> + rstate =
> > to_rcar_crtc_state(rcrtc->crtc.state); +
> > + if (!(rstate->outputs & dpad_mask))
> > + doflr |= doflr_values[i];> +}
> > +
> > + rcar_du_group_write(rgrp, DOFLR, doflr);
> > +}
> > +
> >
> > int rcar_du_group_set_routing(struct rcar_du_group *rgrp)
> > {
> >
> > struct rcar_du_device *rcdu = rgrp->dev;
> >
> > @@ -306,5 +346,7 @@ int rcar_du_group_set_routing(struct rcar_du_group
> > *rgrp)>
> > rcar_du_group_write(rgrp, DORCR, dorcr);
> >
> > + rcar_du_group_set_output_levels(rgrp);
>
> Shouldn't this be:
>
> rcar_du_group_set_dpad_levels()
>
> Anyway - that's just bikeshedding - I'll leave the decision (even if
> that's keeping this as is) to you.
Good idea, I'll rename the function.
> > +
> >
> > return rcar_du_set_dpad0_vsp1_routing(rgrp->dev);
> >
> > }
--
Regards,
Laurent Pinchart
Re: [PATCH v2 07/16] drm: rcar-du: Use LVDS PLL clock as dot clock when possible
Hi Morimoto-san,
On Tuesday, 27 November 2018 02:44:58 EET Kuninori Morimoto wrote:
> Hi Laurent
>
> Sorry for super late response.
> I got opinion from BSP team about this patch.
No worries. My reply is late too I'm afraid :-S
> > On selected SoCs, the DU can use the clock output by the LVDS encoder
> > PLL as its input dot clock. This feature is optional, but on the D3 and
> > E3 SoC it is often the only way to obtain a precise dot clock frequency,
> > as the other available clocks (CPG-generated clock and external clock)
> > usually have fixed rates.
> >
> > Add a DU model information field to describe which DU channels can use
> > the LVDS PLL output clock as their input clock, and configure clock
> > routing accordingly.
> >
> > This feature is available on H2, M2-W, M2-N, D3 and E3 SoCs, with D3 and
> > E3 being the primary targets. It is left disabled in this commit, and
> > will be enabled per-SoC after careful testing.
> >
> > At the hardware level, clock routing is configured at runtime in two
> > steps, first selecting an internal dot clock between the LVDS PLL clock
> > and the external DOTCLKIN clock, and then selecting between the internal
> > dot clock and the CPG-generated clock. The first part requires stopping
> > the whole DU group in order for the change to take effect, thus causing
> > flickering on the screen. For this reason we currently hardcode the
> > clock source to the LVDS PLL clock if available, and allow flicker-free
> > selection of the external DOTCLKIN clock or CPG-generated clock
> > otherwise. A more dynamic clock selection process can be implemented
> > later if the need arises.
> >
> > Signed-off-by: Laurent Pinchart
> >
> > Tested-by: Jacopo Mondi
> > ---
>
> (snip)
>
> > + didsr = DIDSR_CODE;
> > + for (i = 0; i < num_crtcs; ++i, ++rcrtc) {
> > + if (rcdu->info->lvds_clk_mask & BIT(rcrtc->index))
> > + didsr |= DIDSR_LCDS_LVDS0(i)
> > + | DIDSR_PDCS_CLK(i, 0);
> > + else
> > + didsr |= DIDSR_LCDS_DCLKIN(i)
> > + | DIDSR_PDCS_CLK(i, 0);
> > + }
>
> Here, this is for DU pin settings, and fixed for
>
> DU_DOTCLKIN0 -> DU0
> DU_DOTCLKIN1 -> DU1
>
> But on E3 (Ebisu) board, it has only DU_DOTCLKIN0.
> We might use like this
>
> DU_DOTCLKIN0 -> DU0
> DU_DOTCLKIN0 -> DU1
>
> It is possible to adjust to this situation ?
> DIDSR :: PDCSn allows only 0
I think this would make sense. I'm not sure how to implement that, but I'll
give it a try. What is the priority ?
--
Regards,
Laurent Pinchart
Re: [PATCH] clk: vc5: Add suspend/resume support
Hi Marek,
On Wednesday, 5 December 2018 14:29:22 EET Marek Vasut wrote:
> On 12/05/2018 06:21 AM, Laurent Pinchart wrote:
> > On Wednesday, 5 December 2018 01:48:01 EET Marek Vasut wrote:
> >> On 12/04/2018 09:52 PM, Stephen Boyd wrote:
> >>> Quoting Marek Vasut (2018-12-04 10:27:21)
> >>>
> >>>> diff --git a/drivers/clk/clk-versaclock5.c
> >>>> b/drivers/clk/clk-versaclock5.c
> >>>> index decffb3826ec..ac90fb36af1a 100644
> >>>> --- a/drivers/clk/clk-versaclock5.c
> >>>> +++ b/drivers/clk/clk-versaclock5.c
> >>>> @@ -906,6 +906,39 @@ static int vc5_remove(struct i2c_client *client)
> >>>> return 0;
> >>>> }
> >>>>
> >>>> +#ifdef CONFIG_PM_SLEEP
> >>>> +static int vc5_suspend(struct device *dev)
> >>>
> >>> Please mark as __maybe_unused and drop the #ifdef CONFIG_PM_SLEEP
> >>>
> >>>> +{
> >>>> + struct vc5_driver_data *vc5 = dev_get_drvdata(dev);
> >>>> + int ret;
> >>>> +
> >>>> + ret = regcache_sync(vc5->regmap);
> >>>> + if (ret != 0) {
> >>>> + dev_err(dev, "Failed to save register map: %d\n", ret);
> >>>> + return ret;
> >>>
> >>> Do we need to block suspend if we can't save the register map away? Or
> >>> can we just throw up our hands and not restore on resume?
> >>
> >> Some hardware will fail on resume, so I'd say -- yes ?
> >
> > But why do you need to sync on suspend in the first place ? What could
> > cause the map to be dirty at this stage, and require syncing before
> > suspend, that couldn't work with the sync be delayed to resume time ?
>
> Possibly a configuration coming from eg. bootloader time , or some other
> configuration not done by Linux.
I still don't get it. As far as I know, regcache_sync() will write the content
of the regmap to the hardware, not the other way around. You call it at resume
time, so the hardware should be programmed properly, regardless of what the
boot loader or the firmware does when resuming.
Could you please explain why this is needed at suspend time ?
--
Regards,
Laurent Pinchart
Re: [PATCH 1/2] ARM: dts: stout: Convert to new LVDS DT bindings
Hi Marek,
On Wednesday, 5 December 2018 03:10:18 EET Marek Vasut wrote:
> On 12/03/2018 11:48 PM, Laurent Pinchart wrote:
> > On Tuesday, 4 December 2018 00:24:32 EET Marek Vasut wrote:
> >> On 12/03/2018 10:48 PM, Laurent Pinchart wrote:
> >>> On Monday, 3 December 2018 17:12:41 EET Geert Uytterhoeven wrote:
> >>>> As of commit 6d2ca85279becdff ("dt-bindings: display: renesas:
> >>>> Deprecate LVDS support in the DU bindings"), the internal LVDS encoder
> >>>> has DT bindings separate from the DU. The Lager device tree was ported
> >>>> over to the new model, but the Stout device tree was forgotten.
> >>>>
> >>>> Fixes: 15a1ff30d8f9bd83 ("ARM: dts: r8a7790: Convert to new LVDS DT
> >>>> bindings") Signed-off-by: Geert Uytterhoeven
> >>>> ---
> >>>> Compile-tested only.
> >>>
> >>> I can't test the patch either but it looks fine to me.
> >>>
> >>> Reviewed-by: Laurent Pinchart
> >>>
> >>> I assume you will send this directly to Simon, so I don't plan to take
> >>> the patch in my tree.
> >>
> >> I have a Stout if you need me to test something.
> >
> > Could you test HDMI output ? We just need to ensure that everything is
> > probed correctly, so display anything on the HDMI output will do.
>
> Sure, can you push me a branch somewhere, so I don't have to hunt down
> patches ?
Only this patch should be needed. You can get it from https://
patchwork.kernel.org/patch/10709781/ and apply it on top of v4.19. Please
don't use v4.20-rc is there's one missing regression fix there (it has been
merged in the media tree and should make it to the final release).
--
Regards,
Laurent Pinchart
Re: [PATCH] clk: vc5: Add suspend/resume support
Hi Marek,
On Wednesday, 5 December 2018 01:48:01 EET Marek Vasut wrote:
> On 12/04/2018 09:52 PM, Stephen Boyd wrote:
> > Quoting Marek Vasut (2018-12-04 10:27:21)
> >
> >> diff --git a/drivers/clk/clk-versaclock5.c
> >> b/drivers/clk/clk-versaclock5.c
> >> index decffb3826ec..ac90fb36af1a 100644
> >> --- a/drivers/clk/clk-versaclock5.c
> >> +++ b/drivers/clk/clk-versaclock5.c
> >> @@ -906,6 +906,39 @@ static int vc5_remove(struct i2c_client *client)
> >>
> >> return 0;
> >>
> >> }
> >>
> >> +#ifdef CONFIG_PM_SLEEP
> >> +static int vc5_suspend(struct device *dev)
> >
> > Please mark as __maybe_unused and drop the #ifdef CONFIG_PM_SLEEP
> >
> >> +{
> >> + struct vc5_driver_data *vc5 = dev_get_drvdata(dev);
> >> + int ret;
> >> +
> >> + ret = regcache_sync(vc5->regmap);
> >> + if (ret != 0) {
> >> + dev_err(dev, "Failed to save register map: %d\n", ret);
> >> + return ret;
> >
> > Do we need to block suspend if we can't save the register map away? Or
> > can we just throw up our hands and not restore on resume?
>
> Some hardware will fail on resume, so I'd say -- yes ?
But why do you need to sync on suspend in the first place ? What could cause
the map to be dirty at this stage, and require syncing before suspend, that
couldn't work with the sync be delayed to resume time ?
> The rest is fixed.
>
> >> + }
> >> + regcache_cache_only(vc5->regmap, true);
> >> + regcache_mark_dirty(vc5->regmap);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int vc5_resume(struct device *dev)
> >> +{
> >> + struct vc5_driver_data *vc5 = dev_get_drvdata(dev);
> >> + int ret;
> >> +
> >> + regcache_cache_only(vc5->regmap, false);
> >> + ret = regcache_sync(vc5->regmap);
> >> + if (ret != 0) {
> >> + dev_err(dev, "Failed to restore register map: %d\n",
> >> ret);
> >> + return ret;
> >> + }
> >
> > Simplify to
> >
> > if (ret)
> >
> > dev_err()
> >
> > retun ret;
> >
> >> +
> >> + return 0;
> >> +}
> >> +#endif
> >> +
> >>
> >> static const struct vc5_chip_info idt_5p49v5923_info = {
> >>
> >> .model = IDT_VC5_5P49V5923,
> >> .clk_fod_cnt = 2,
--
Regards,
Laurent Pinchart
Re: [PATCH v2] drm: rcar-du: dw-hdmi: Reject modes with a too high clock frequency
Hi Geert,
On Tuesday, 4 December 2018 21:45:10 EET Geert Uytterhoeven wrote:
> On Tue, Dec 4, 2018 at 7:51 PM Laurent Pinchart wrote:
> > On Tuesday, 4 December 2018 20:42:53 EET Geert Uytterhoeven wrote:
> > > On Tue, Dec 4, 2018 at 7:12 PM Laurent Pinchart wrote:
> > > > On Tuesday, 4 December 2018 19:30:25 EET Geert Uytterhoeven wrote:
> > > >> On Tue, Dec 4, 2018 at 5:36 PM Laurent Pinchart wrote:
> > > >>> Implement a .mode_valid() handler in the R-Car glue layer to reject
> > > >>> modes with an unsupported clock frequency.
> > > >>>
> > > >>> Signed-off-by: Laurent Pinchart
> > > >>>
> > > >>
> > > >> Thanks for your patch!
> > > >>
> > > >>> --- a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
> > > >>> +++ b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
> > > >>> @@ -35,6 +35,20 @@ static const struct rcar_hdmi_phy_params
> > > >>> rcar_hdmi_phy_params[] = {
> > > >>>
> > > >>> { ~0UL, 0x, 0x, 0x },
> > > >>>
> > > >>> };
> > > >>>
> > > >>> +static enum drm_mode_status
> > > >>> +rcar_hdmi_mode_valid(struct drm_connector *connector,
> > > >>> +const struct drm_display_mode *mode)
> > > >>> +{
> > > >>> + /*
> > > >>> +* The maximum supported clock frequency is 297 MHz, as
> > > >>> shown
> > > >>> in the PHY
> > > >>> +* parameters table.
> > > >>> +*/
> > > >>> + if (mode->clock > 297000)
> > > >>> + return MODE_CLOCK_HIGH;
> > > >>
> > > >> Perhaps you need a check for the lower limit (25 MHz), too?
> > > >
> > > > There's no lower limit implied by the rcar_hdmi_phy_params table.
> > >
> > > Oh, you mean the table in the driver, not a table in the Hardware User's
> > > Manual?
> >
> > Correct, I mean the table in the driver. This patch was prompted by an
> > error returned from rcar_hdmi_phy_configure() when the mode frequency was
> > too high, making mode setting failed. I've thus added a .mode_valid()
> > handler to ensure that invalid modes don't get exposed to upper layers,
> > fixing such use cases as fbvon on a 4K monitor (where the fbcon was
> > picking a mode advertised as supported by the driver while its frequency
> > was too high).
> >
> > > That's why I couldn't find the table, but only a short notice in the
> > > HDMI section of the Hardware User's Manual, stating:
> > >
> > > Pixel clock from 25MHz up to 297MHz
> >
> > Well, the IP core vendor doesn't allow us to submit patches based on the
> > content of non-public documentation, so I'm afraid I won't sign such a
> > patch without being given explicit permission. It's a very stupid game
> > really, but I don't set the rules :-(
>
> https://en.wikipedia.org/wiki/HDMI claims 25 MHz is the minimum TMDS rate
> for HDMI anyway. Anything below that needs to use pixel replication.
>
> So you can reject < 25 MHz for sure.
That should then be performed in the common dw_hdmi_bridge_mode_valid()
handler, in drivers/gpu/drm/bridge/synopsys/dw-hdmi.c.
--
Regards,
Laurent Pinchart
Re: [PATCH v2] drm: rcar-du: dw-hdmi: Reject modes with a too high clock frequency
Hi Geert,
On Tuesday, 4 December 2018 20:42:53 EET Geert Uytterhoeven wrote:
> On Tue, Dec 4, 2018 at 7:12 PM Laurent Pinchart wrote:
> > On Tuesday, 4 December 2018 19:30:25 EET Geert Uytterhoeven wrote:
> >> On Tue, Dec 4, 2018 at 5:36 PM Laurent Pinchart wrote:
> >>> Implement a .mode_valid() handler in the R-Car glue layer to reject
> >>> modes with an unsupported clock frequency.
> >>>
> >>> Signed-off-by: Laurent Pinchart
> >>>
> >>
> >> Thanks for your patch!
> >>
> >>> --- a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
> >>> +++ b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
> >>> @@ -35,6 +35,20 @@ static const struct rcar_hdmi_phy_params
> >>> rcar_hdmi_phy_params[] = {
> >>> { ~0UL, 0x, 0x, 0x },
> >>> };
> >>>
> >>> +static enum drm_mode_status
> >>> +rcar_hdmi_mode_valid(struct drm_connector *connector,
> >>> +const struct drm_display_mode *mode)
> >>> +{
> >>> + /*
> >>> +* The maximum supported clock frequency is 297 MHz, as shown
> >>> in the PHY
> >>> +* parameters table.
> >>> +*/
> >>> + if (mode->clock > 297000)
> >>> + return MODE_CLOCK_HIGH;
> >>
> >> Perhaps you need a check for the lower limit (25 MHz), too?
> >
> > There's no lower limit implied by the rcar_hdmi_phy_params table.
>
> Oh, you mean the table in the driver, not a table in the Hardware User's
> Manual?
Correct, I mean the table in the driver. This patch was prompted by an error
returned from rcar_hdmi_phy_configure() when the mode frequency was too high,
making mode setting failed. I've thus added a .mode_valid() handler to ensure
that invalid modes don't get exposed to upper layers, fixing such use cases as
fbvon on a 4K monitor (where the fbcon was picking a mode advertised as
supported by the driver while its frequency was too high).
> That's why I couldn't find the table, but only a short notice in the HDMI
> section of the Hardware User's Manual, stating:
>
> Pixel clock from 25MHz up to 297MHz
Well, the IP core vendor doesn't allow us to submit patches based on the
content of non-public documentation, so I'm afraid I won't sign such a patch
without being given explicit permission. It's a very stupid game really, but I
don't set the rules :-(
--
Regards,
Laurent Pinchart
Re: [PATCH v2] drm: rcar-du: dw-hdmi: Reject modes with a too high clock frequency
Hi Geert,
On Tuesday, 4 December 2018 19:30:25 EET Geert Uytterhoeven wrote:
> On Tue, Dec 4, 2018 at 5:36 PM Laurent Pinchart wrote:
> > Implement a .mode_valid() handler in the R-Car glue layer to reject
> > modes with an unsupported clock frequency.
> >
> > Signed-off-by: Laurent Pinchart
> >
>
> Thanks for your patch!
>
> > --- a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
> > @@ -35,6 +35,20 @@ static const struct rcar_hdmi_phy_params
> > rcar_hdmi_phy_params[] = {
> > { ~0UL, 0x, 0x, 0x },
> > };
> >
> > +static enum drm_mode_status
> > +rcar_hdmi_mode_valid(struct drm_connector *connector,
> > +const struct drm_display_mode *mode)
> > +{
> > + /*
> > +* The maximum supported clock frequency is 297 MHz, as shown in
> > the PHY +* parameters table.
> > +*/
> > + if (mode->clock > 297000)
> > + return MODE_CLOCK_HIGH;
>
> Perhaps you need a check for the lower limit (25 MHz), too?
There's no lower limit implied by the rcar_hdmi_phy_params table.
> > +
> > + return MODE_OK;
> > +}
--
Regards,
Laurent Pinchart
Re: [PATCH 4/5] arm64: dts: renesas: r8a77995: draak: Add backlight
Hi Simon,
Could you please consider taking this patch in your tree ? It's independent
from the rest of the series.
On Sunday, 25 November 2018 16:40:30 EET Laurent Pinchart wrote:
> Add the backlight device for the LVDS1 output, in preparation for panel
> support.
>
> Signed-off-by: Laurent Pinchart
> ---
> .../arm64/boot/dts/renesas/r8a77995-draak.dts | 20 +++
> 1 file changed, 20 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts index
> 2405eaad0296..cd067319e6f3 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> @@ -24,6 +24,17 @@
> stdout-path = "serial0:115200n8";
> };
>
> + backlight: backlight {
> + compatible = "pwm-backlight";
> + pwms = <&pwm1 0 5>;
> +
> + brightness-levels = <256 128 64 16 8 4 0>;
> + default-brightness-level = <6>;
> +
> + power-supply = <®_12p0v>;
> + enable-gpios = <&gpio4 0 GPIO_ACTIVE_HIGH>;
> + };
> +
> composite-in {
> compatible = "composite-video-connector";
>
> @@ -104,6 +115,15 @@
> regulator-always-on;
> };
>
> + reg_12p0v: regulator1 {
> + compatible = "regulator-fixed";
> + regulator-name = "D12.0V";
> + regulator-min-microvolt = <1200>;
> + regulator-max-microvolt = <1200>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> +
> vga {
> compatible = "vga-connector";
--
Regards,
Laurent Pinchart
[PATCH v2] drm: rcar-du: dw-hdmi: Reject modes with a too high clock frequency
Implement a .mode_valid() handler in the R-Car glue layer to reject
modes with an unsupported clock frequency.
Signed-off-by: Laurent Pinchart
---
drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c | 15 +++
1 file changed, 15 insertions(+)
Changes since v1:
- Add a comment to explain where the limit comes from
diff --git a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
index 75490a3e0a2a..603bb340e8cf 100644
--- a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
+++ b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
@@ -35,6 +35,20 @@ static const struct rcar_hdmi_phy_params
rcar_hdmi_phy_params[] = {
{ ~0UL, 0x, 0x, 0x },
};
+static enum drm_mode_status
+rcar_hdmi_mode_valid(struct drm_connector *connector,
+const struct drm_display_mode *mode)
+{
+ /*
+* The maximum supported clock frequency is 297 MHz, as shown in the PHY
+* parameters table.
+*/
+ if (mode->clock > 297000)
+ return MODE_CLOCK_HIGH;
+
+ return MODE_OK;
+}
+
static int rcar_hdmi_phy_configure(struct dw_hdmi *hdmi,
const struct dw_hdmi_plat_data *pdata,
unsigned long mpixelclock)
@@ -59,6 +73,7 @@ static int rcar_hdmi_phy_configure(struct dw_hdmi *hdmi,
}
static const struct dw_hdmi_plat_data rcar_dw_hdmi_plat_data = {
+ .mode_valid = rcar_hdmi_mode_valid,
.configure_phy = rcar_hdmi_phy_configure,
};
--
Regards,
Laurent Pinchart
Re: [PATCH 1/2] ARM: dts: stout: Convert to new LVDS DT bindings
Hi Marek,
On Tuesday, 4 December 2018 00:24:32 EET Marek Vasut wrote:
> On 12/03/2018 10:48 PM, Laurent Pinchart wrote:
> > On Monday, 3 December 2018 17:12:41 EET Geert Uytterhoeven wrote:
> >> As of commit 6d2ca85279becdff ("dt-bindings: display: renesas: Deprecate
> >> LVDS support in the DU bindings"), the internal LVDS encoder has DT
> >> bindings separate from the DU. The Lager device tree was ported over to
> >> the new model, but the Stout device tree was forgotten.
> >>
> >> Fixes: 15a1ff30d8f9bd83 ("ARM: dts: r8a7790: Convert to new LVDS DT
> >> bindings") Signed-off-by: Geert Uytterhoeven
> >> ---
> >> Compile-tested only.
> >
> > I can't test the patch either but it looks fine to me.
> >
> > Reviewed-by: Laurent Pinchart
> >
> > I assume you will send this directly to Simon, so I don't plan to take the
> > patch in my tree.
>
> I have a Stout if you need me to test something.
Could you test HDMI output ? We just need to ensure that everything is probed
correctly, so display anything on the HDMI output will do.
--
Regards,
Laurent Pinchart
Re: [PATCH 2/2] arm64: dts: renesas: r8a7796: salvator-xs: Convert to new LVDS DT bindings
Hi Geert,
Thank you for the patch.
On Monday, 3 December 2018 17:12:42 EET Geert Uytterhoeven wrote:
> As of commit 6d2ca85279becdff ("dt-bindings: display: renesas: Deprecate
> LVDS support in the DU bindings"), the internal LVDS encoder has DT
> bindings separate from the DU. The device trees for all R-Car H3 and
> M3-W development boards were ported over to the new model, but
> Salvator-XS boards equipped with an R-Car M3-W SoC were forgotten.
>
> Fixes: 58e8ed2ee9abe718 ("arm64: dts: renesas: Convert to new LVDS DT
> bindings") Signed-off-by: Geert Uytterhoeven
> ---
> Compile-tested only.
I can't test the patch either but it looks fine to me.
Reviewed-by: Laurent Pinchart
I assume you will send this directly to Simon, so I don't plan to take the
patch in my tree.
> ---
> arch/arm64/boot/dts/renesas/r8a7796-salvator-xs.dts | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a7796-salvator-xs.dts
> b/arch/arm64/boot/dts/renesas/r8a7796-salvator-xs.dts index
> 8860be65342e4f38..31f12059355ee02b 100644
> --- a/arch/arm64/boot/dts/renesas/r8a7796-salvator-xs.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a7796-salvator-xs.dts
> @@ -29,11 +29,10 @@
> clocks = <&cpg CPG_MOD 724>,
><&cpg CPG_MOD 723>,
><&cpg CPG_MOD 722>,
> - <&cpg CPG_MOD 727>,
><&versaclock6 1>,
><&x21_clk>,
><&versaclock6 2>;
> - clock-names = "du.0", "du.1", "du.2", "lvds.0",
> + clock-names = "du.0", "du.1", "du.2",
> "dclkin.0", "dclkin.1", "dclkin.2";
> };
--
Regards,
Laurent Pinchart
Re: [PATCH 1/2] ARM: dts: stout: Convert to new LVDS DT bindings
Hi Geert,
Thank you for the patch.
On Monday, 3 December 2018 17:12:41 EET Geert Uytterhoeven wrote:
> As of commit 6d2ca85279becdff ("dt-bindings: display: renesas: Deprecate
> LVDS support in the DU bindings"), the internal LVDS encoder has DT
> bindings separate from the DU. The Lager device tree was ported over to
> the new model, but the Stout device tree was forgotten.
>
> Fixes: 15a1ff30d8f9bd83 ("ARM: dts: r8a7790: Convert to new LVDS DT
> bindings") Signed-off-by: Geert Uytterhoeven
> ---
> Compile-tested only.
I can't test the patch either but it looks fine to me.
Reviewed-by: Laurent Pinchart
I assume you will send this directly to Simon, so I don't plan to take the
patch in my tree.
> ---
> arch/arm/boot/dts/r8a7790-stout.dts | 15 ---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/boot/dts/r8a7790-stout.dts
> b/arch/arm/boot/dts/r8a7790-stout.dts index
> 629da4cee1b971d6..7a7d3b84d1a6b21d 100644
> --- a/arch/arm/boot/dts/r8a7790-stout.dts
> +++ b/arch/arm/boot/dts/r8a7790-stout.dts
> @@ -94,9 +94,8 @@
> status = "okay";
>
> clocks = <&cpg CPG_MOD 724>, <&cpg CPG_MOD 723>, <&cpg CPG_MOD 722>,
> - <&cpg CPG_MOD 726>, <&cpg CPG_MOD 725>,
><&osc1_clk>;
> - clock-names = "du.0", "du.1", "du.2", "lvds.0", "lvds.1", "dclkin.0";
> + clock-names = "du.0", "du.1", "du.2", "dclkin.0";
>
> ports {
> port@0 {
> @@ -104,11 +103,21 @@
> remote-endpoint = <&adv7511_in>;
> };
> };
> + };
> +};
> +
> +&lvds0 {
> + ports {
> port@1 {
> lvds_connector0: endpoint {
> };
> };
> - port@2 {
> + };
> +};
> +
> +&lvds1 {
> + ports {
> + port@1 {
> lvds_connector1: endpoint {
> };
> };
--
Regards,
Laurent Pinchart
Re: [PATCH v4 1/4] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints
Hi Niklas, Thank you for the patch. On Thursday, 29 November 2018 04:01:44 EET Niklas Söderlund wrote: > The CSI-2 transmitters can use a different number of lanes to transmit > data. Make the data-lanes mandatory for the endpoints that describe the > transmitters as no good default can be set to fallback on. > > Signed-off-by: Niklas Söderlund Reviewed-by: Laurent Pinchart > --- > * Changes since v3 > - Add paragraph to describe the accepted values for the source endpoint > data-lane property. Thanks Jacopo for pointing this out and sorry for > missing this in v2. > * Changes since v2 > - Update paragraph according to Laurents comments. > --- > .../devicetree/bindings/media/i2c/adv748x.txt | 11 ++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt > b/Documentation/devicetree/bindings/media/i2c/adv748x.txt index > 5dddc95f9cc46084..4f91686e54a6b939 100644 > --- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt > +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt > @@ -48,7 +48,16 @@ are numbered as follows. > TXA source 10 > TXB source 11 > > -The digital output port nodes must contain at least one endpoint. > +The digital output port nodes, when present, shall contain at least one > +endpoint. Each of those endpoints shall contain the data-lanes property as > +described in video-interfaces.txt. > + > +Required source endpoint properties: > + - data-lanes: an array of physical data lane indexes > +The accepted value(s) for this property depends on which of the two > +sources are described. For TXA 1, 2 or 4 data lanes can be described > +while for TXB only 1 data lane is valid. See video-interfaces.txt > +for detailed description. > > Ports are optional if they are not connected to anything at the hardware > level. -- Regards, Laurent Pinchart
Re: [PATCH] drm: rcar-du: Convert live DT patches to sugar syntax
Hi Geert,
Thank you for the patch.
On Thursday, 29 November 2018 12:58:50 EET Geert Uytterhoeven wrote:
> Using overlay sugar syntax makes the DTS files easier to read (and
> write).
>
> Overlay syntactic sugar for generating target-path fragments is
> supported by the version of dtc supplied with the kernel since commit
> 50aafd60898a8b3e ("scripts/dtc: Update to upstream version
> v1.4.6-21-g84e414b0b5bc").
>
> Signed-off-by: Geert Uytterhoeven
> ---
> The generated dtbs before/after are identical.
I was going to ask whether you had tested the patch, but this should be good
enough :-)
The result is nicer to read, thanks.
Reviewed-by: Laurent Pinchart
and taken in my tree.
> ---
> .../drm/rcar-du/rcar_du_of_lvds_r8a7790.dts | 93 +--
> .../drm/rcar-du/rcar_du_of_lvds_r8a7791.dts | 53 +--
> .../drm/rcar-du/rcar_du_of_lvds_r8a7793.dts | 53 +--
> .../drm/rcar-du/rcar_du_of_lvds_r8a7795.dts | 53 +--
> .../drm/rcar-du/rcar_du_of_lvds_r8a7796.dts | 53 +--
> 5 files changed, 135 insertions(+), 170 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dts
> b/drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dts index
> 579753e04f3b3712..8bee4e787a0ab4eb 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dts
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dts
> @@ -7,70 +7,63 @@
>
> /dts-v1/;
> /plugin/;
> -/ {
> - fragment@0 {
> - target-path = "/";
> - __overlay__ {
> - #address-cells = <2>;
> - #size-cells = <2>;
>
> - lvds@feb9 {
> - compatible = "renesas,r8a7790-lvds";
> - reg = <0 0xfeb9 0 0x1c>;
> +&{/} {
> + #address-cells = <2>;
> + #size-cells = <2>;
>
> - ports {
> - #address-cells = <1>;
> - #size-cells = <0>;
> + lvds@feb9 {
> + compatible = "renesas,r8a7790-lvds";
> + reg = <0 0xfeb9 0 0x1c>;
>
> - port@0 {
> - reg = <0>;
> - lvds0_input: endpoint {
> - };
> - };
> - port@1 {
> - reg = <1>;
> - lvds0_out: endpoint {
> - };
> - };
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + lvds0_input: endpoint {
> };
> };
> -
> - lvds@feb94000 {
> - compatible = "renesas,r8a7790-lvds";
> - reg = <0 0xfeb94000 0 0x1c>;
> -
> - ports {
> - #address-cells = <1>;
> - #size-cells = <0>;
> -
> - port@0 {
> - reg = <0>;
> - lvds1_input: endpoint {
> - };
> - };
> - port@1 {
> - reg = <1>;
> - lvds1_out: endpoint {
> - };
> - };
> + port@1 {
> + reg = <1>;
> + lvds0_out: endpoint {
> };
> };
> };
> };
>
> - fragment@1 {
> - target-path = "/display@feb0/ports";
> - __overlay__ {
> - port@1 {
> - endpoint {
> - remote-endpoint = <&lvds0_input>;
> + lvds@feb94000 {
> + compatible = "renesas,r8a7790-lvds";
> + reg = <0 0x
Re: Issue with enabling VSP source on rcar gen2 koelsch board
Hi Biju, On Thursday, 29 November 2018 10:39:00 EET Biju Das wrote: > > Subject: Re: Issue with enabling VSP source on rcar gen2 koelsch board > > On Wednesday, 28 November 2018 15:20:58 EET Biju Das wrote: > >> Hi all, > >> > >> On the past, I have tested vsp source on rcar gen2 koelsch board, > >> using the patches series below(Apart from the below patch series, I > >> have enabled "CONFIG_DRM_RCAR_VSP=y") > >> https://git.linuxtv.org/pinchartl/media.git/log/?h=drm/du/panels > >> > >> 1) [HACK] ARM: shmobile: r8a7791: Link the VSP1 and DU > >> 2) [HACK] v4l: vsp1: Disable UAPI for VSP1D > >> 3) [HACK] drm: rcar-du: Enable VSP source support for R8A7791 > >> > >> But on the latest renesas-dev branch looks like it is broken > > > > That doesn't surprise me too much as I don't actively maintain that code. > > > > For my information, what are your use cases ? Do you think this approach > > should be pursued and patches merged upstream ? The amount of work will > > be quite high though, given that it introduces a new userspace API, we > > will have to publish an open-source implementation in an X.org or Wayland > > driver. > > Basically as part of RZ-G1/M VSP upstreaming(Linux 4.15 kernel) ,previously > I have executed the below test cases > > 1) vsp test suite:- based on the instruction present on > "http://git.ideasonboard.com/renesas/vsp-tests.git"; > > 2) vspd testing:- based on the instruction present on > "https://git.linuxtv.org/pinchartl/media.git/log/?h=drm/du/panels"; > > Both the test cases were working fine with 4.15 kernel. > > Now as part of RZ/G1N vsp upstreaming, I rebased to latest branch from > renesas-dev , and executed the same test case > > Only test cases in vsp test suite is working. the vspd test case is broken. Could you please post the tests output and log ? > So I am wondering how do we expose the r-car gen2 SoC feature "VSPD" to > the end user? Isn't the VSPD already exposed to the user as a V4L2 device on Gen2 ? -- Regards, Laurent Pinchart
Re: [PATCH 00/13] clk: renesas: Miscellaneous fixes
Hi Geert, On Thursday, 29 November 2018 12:49:55 EET Geert Uytterhoeven wrote: > Hi Mike, Stephen, Laurent, Kieran, > > This patch series contains several fixes for the Renesas Clock drivers > and DT bindings, and a small simplification: > - Removal of non-existent clocks, > - Addition of the CPEX clocks, which can be used a source for a timer > (CMT1), > - Correction of DU parent clocks. > Most of these have been found by skimming the Hardware Manual Errata. > > This has been boot-tested on Salvator-X(S) (R-Car H3, M3-W, M3-N), Eagle > (R-Car V3M), and Draak (R-Car D3), except for the DU parts. > Testing of the DU on R-Car D3 and E3 would be appreciated, as the DU > driver may have a workaround for the incorrect parent clock rates. As explained in my review of 09/13, I believe the DU clock patches to both be correct and have no effect for D3 and E3. No action is needed on the DU side, there is no workaround for the incorrect frequency. > I intend to queue this in clk-renesas-for-v4.21. > > Thanks! > > Geert Uytterhoeven (12): > dt-bindings: clock: r8a7795: Remove CSIREF clock > dt-bindings: clock: r8a7796: Remove CSIREF clock > clk: renesas: r8a774a1: Add CPEX clock > clk: renesas: r8a7795: Add CPEX clock > clk: renesas: r8a7796: Add CPEX clock > clk: renesas: r8a77965: Add CPEX clock > clk: renesas: r8a77970: Add CPEX clock > clk: renesas: r8a77995: Correct parent clock of DU > clk: renesas: r8a77995: Remove non-existent VIN5-7 module clocks > clk: renesas: r8a77995: Remove non-existent SSP clocks > clk: renesas: r8a77995: Add missing CPEX clock > clk: renesas: r8a77995: Simplify PLL3 multiplier/divider > > Takeshi Kihara (1): > clk: renesas: r8a77990: Correct parent clock of DU > > drivers/clk/renesas/r8a774a1-cpg-mssr.c | 1 + > drivers/clk/renesas/r8a7795-cpg-mssr.c| 1 + > drivers/clk/renesas/r8a7796-cpg-mssr.c| 1 + > drivers/clk/renesas/r8a77965-cpg-mssr.c | 1 + > drivers/clk/renesas/r8a77970-cpg-mssr.c | 1 + > drivers/clk/renesas/r8a77990-cpg-mssr.c | 4 ++-- > drivers/clk/renesas/r8a77995-cpg-mssr.c | 15 ++- > include/dt-bindings/clock/r8a7795-cpg-mssr.h | 2 +- > include/dt-bindings/clock/r8a7796-cpg-mssr.h | 2 +- > include/dt-bindings/clock/r8a77995-cpg-mssr.h | 5 +++-- > 10 files changed, 18 insertions(+), 15 deletions(-) -- Regards, Laurent Pinchart
Re: [PATCH 09/13] clk: renesas: r8a77995: Correct parent clock of DU
Hi Geert,
Thank you for the patch.
On Thursday, 29 November 2018 12:50:04 EET Geert Uytterhoeven wrote:
> According to the R-Car Gen3 Hardware Manual Rev 1.00, the parent clock
> of the DU module clocks on R-Car D3 is S1D1.
>
> Fixes: d71e851d82c6cfe5 ("clk: renesas: cpg-mssr: Add R8A77995 support")
> Signed-off-by: Geert Uytterhoeven
Reviewed-by: Laurent Pinchart
The patch has no effect on the DU operation as the rcar-du driver hardcodes a
different clock source for the dot clock. However, after hacking the driver to
hardcode usage of this clock, operation off the LVDS output is improved. I
thus believe this change to be correct in addition to not having any effect
with the current mainline code :-)
Tested-by: Laurent Pinchart
> ---
> drivers/clk/renesas/r8a77995-cpg-mssr.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/renesas/r8a77995-cpg-mssr.c
> b/drivers/clk/renesas/r8a77995-cpg-mssr.c index
> 47e60e3dbe05ff18..ad95dc225e9c039a 100644
> --- a/drivers/clk/renesas/r8a77995-cpg-mssr.c
> +++ b/drivers/clk/renesas/r8a77995-cpg-mssr.c
> @@ -146,8 +146,8 @@ static const struct mssr_mod_clk r8a77995_mod_clks[]
> __initconst = { DEF_MOD("vspbs", 627, R8A77995_CLK_S0D1),
> DEF_MOD("ehci0", 703, R8A77995_CLK_S3D2),
> DEF_MOD("hsusb", 704, R8A77995_CLK_S3D2),
> - DEF_MOD("du1", 723, R8A77995_CLK_S2D1),
> - DEF_MOD("du0", 724, R8A77995_CLK_S2D1),
> + DEF_MOD("du1", 723, R8A77995_CLK_S1D1),
> + DEF_MOD("du0", 724, R8A77995_CLK_S1D1),
> DEF_MOD("lvds", 727, R8A77995_CLK_S2D1),
> DEF_MOD("vin7", 804, R8A77995_CLK_S1D2),
> DEF_MOD("vin6", 805, R8A77995_CLK_S1D2),
--
Regards,
Laurent Pinchart
Re: [PATCH 08/13] clk: renesas: r8a77990: Correct parent clock of DU
Hi Geert,
Thank you for the patch.
On Thursday, 29 November 2018 12:50:03 EET Geert Uytterhoeven wrote:
> From: Takeshi Kihara
>
> According to the R-Car Gen3 Hardware Manual Rev 1.00, the parent clock
> of the DU module clocks on R-Car E3 is S1D1.
>
> Signed-off-by: Takeshi Kihara
> Fixes: 3570a2af473789c5 ("clk: renesas: cpg-mssr: Add support for R-Car E3")
> Signed-off-by: Geert Uytterhoeven
Reviewed-by: Laurent Pinchart
> ---
> drivers/clk/renesas/r8a77990-cpg-mssr.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/renesas/r8a77990-cpg-mssr.c
> b/drivers/clk/renesas/r8a77990-cpg-mssr.c index
> 9eb80180eea0b1a6..9a278c75c918cfa8 100644
> --- a/drivers/clk/renesas/r8a77990-cpg-mssr.c
> +++ b/drivers/clk/renesas/r8a77990-cpg-mssr.c
> @@ -183,8 +183,8 @@ static const struct mssr_mod_clk r8a77990_mod_clks[]
> __initconst = { DEF_MOD("ehci0", 703, R8A77990_CLK_S3D4),
> DEF_MOD("hsusb", 704, R8A77990_CLK_S3D4),
> DEF_MOD("csi40", 716, R8A77990_CLK_CSI0),
> - DEF_MOD("du1", 723, R8A77990_CLK_S2D1),
> - DEF_MOD("du0", 724, R8A77990_CLK_S2D1),
> + DEF_MOD("du1", 723, R8A77990_CLK_S1D1),
> + DEF_MOD("du0", 724, R8A77990_CLK_S1D1),
> DEF_MOD("lvds", 727, R8A77990_CLK_S2D1),
>
> DEF_MOD("vin5", 806, R8A77990_CLK_S1D2),
--
Regards,
Laurent Pinchart
Re: Issue with enabling VSP source on rcar gen2 koelsch board
Hi Biju, On Wednesday, 28 November 2018 15:20:58 EET Biju Das wrote: > Hi all, > > On the past, I have tested vsp source on rcar gen2 koelsch board, using the > patches series below(Apart from the below patch series, I have enabled > "CONFIG_DRM_RCAR_VSP=y") > https://git.linuxtv.org/pinchartl/media.git/log/?h=drm/du/panels > > 1) [HACK] ARM: shmobile: r8a7791: Link the VSP1 and DU > 2) [HACK] v4l: vsp1: Disable UAPI for VSP1D > 3) [HACK] drm: rcar-du: Enable VSP source support for R8A7791 > > But on the latest renesas-dev branch looks like it is broken That doesn't surprise me too much as I don't actively maintain that code. For my information, what are your use cases ? Do you think this approach should be pursued and patches merged upstream ? The amount of work will be quite high though, given that it introduces a new userspace API, we will have to publish an open-source implementation in an X.org or Wayland driver. > The issue is kernel doesn't boot, if I connect display panel. Looks like it > is hanging in register_framebuffer. > > [5.344412] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). > [5.364250] [drm] No driver support for vblank timestamp query. > > Have any one noticed this issue? This usually means that the kernel crashes with the console lock taken, preventing the oops from being printed. As a debugging hack you can comment out the locking calls in console_lock(), console_trylock() and console_unlock() (kernel/printk/printk.c). -- Regards, Laurent Pinchart
Re: [PATCH] drm: rcar-du: Fix DU3 start/stop on M3-N
Hello Hoan-san,
On Monday, 26 November 2018 11:46:56 EET Hoan wrote:
> Dear Laurent-san
>
> Thank you for your comments on my patches! I understand.
>
> With this patch, the problem has been improved.
>
> CC Simon-san
>
> If you wait a little longer, the error log will look like this:
There is another display-related regression in v4.20, for which I have posted
https://patchwork.linuxtv.org/patch/53083/. I believe it should fix the issue
you're experienced.
> [2.825800] [drm] No driver support for vblank timestamp query.
> [ 13.027591] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR*
> [CRTC:55:crtc-2] flip_done timed out
> [ 23.267575] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR*
> [CRTC:55:crtc-2] flip_done timed out
> [ 33.507572] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR*
> [CONNECTOR:57:VGA-1] flip_done timed out
> [ 43.747572] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR*
> [PLANE:30:plane-1] flip_done timed out
> [ 53.987572] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR*
> [CRTC:55:crtc-2] flip_done timed out
> [ 53.990386] Console: switching to colour frame buffer device 128x48
> [ 64.227573] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR*
> [CRTC:55:crtc-2] flip_done timed out
> [ 74.467571] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR*
> [CONNECTOR:57:VGA-1] flip_done timed out
> [ 84.707570] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR*
> [PLANE:30:plane-1] flip_done timed out
> [ 94.947573] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR*
> [CRTC:55:crtc-2] flip_done timed out
> [ 95.040503] rcar-du feb0.display: fb0: DRM emulated frame buffer
> device
> [ 95.048144] [drm] Initialized rcar-du 1.0.0 20130110 for
> feb0.display on minor 0
> [ 95.055907] [drm] Device feb0.display probed
> ...
>
> On 2018/11/26 17:21, Simon Horman wrote:
> > On Fri, Nov 23, 2018 at 01:48:08PM +0200, Laurent Pinchart wrote:
> >> Group start/stop is controlled by the DRES and DEN bits of DSYSR0 for
> >> the first group and DSYSR2 for the second group. On most DU instances,
> >> this maps to the first CRTC of the group. On M3-N, however, DU2 doesn't
> >> exist, but DSYSR2 does. There is no CRTC object there that maps to the
> >> correct DSYSR register.
> >>
> >> Commit 9144adc5e5a9 ("drm: rcar-du: Cache DSYSR value to ensure known
> >> initial value") switched group start/stop from using group read/write
> >> access to DSYSR to a CRTC-based API to cache the DSYSR value. While
> >> doing so, it introduced a regression on M3-N by accessing DSYSR3 instead
> >> of DSYSR2 to start/stop the second group.
> >>
> >> To fix this, access the DSYSR register directly through group read/write
> >> if the SoC is missing the first DU channel of the group. Keep using the
> >> rcar_du_crtc_dsysr_clr_set() function otherwise, to retain the DSYSR
> >> caching feature.
> >>
> >> Fixes: 9144adc5e5a9 ("drm: rcar-du: Cache DSYSR value to ensure known
> >> initial value") Reported-by: Hoan Nguyen An
> >> Signed-off-by: Laurent Pinchart
> >>
> >
> > Thanks Laurent,
> >
> > I have confirmed that with this patch applied on top of
> > renesas-devel-20181123-v4.20-rc3 Salvator-XS / M3-N ES1.0
> > boots to user-space when the kernel is compiled using renesas_defconfig.
> >
> > Tested-by: Simon Horman
> >
> >
> > Without this patch applied the boot log looks this:
> >
> > Starting kernel ...
[snip]
--
Regards,
Laurent Pinchart
[PATCH 2/5] drm: rcar-du: Move CRTC outputs bitmask to private CRTC state
The rcar_du_crtc outputs field stores a bitmask of the outputs driven by
the CRTC. This changes based on the configuration requested by
userspace, and is used for the sole purpose of configuring the hardware.
The field thus belongs to the CRTC state. Move it to the
rcar_du_crtc_state structure.
As a result the rcar_du_crtc_route_output() function loses most of its
purpose. In order to remove it, move dpad0_source calculation to
rcar_du_atomic_commit_tail(), until the field gets moved to a state
structure. In order to simplify the rcar_du_group_set_routing()
implementation, we also store the DPAD1 source in a new dpad1_source
field which will move to a state structure with dpad0_source.
Signed-off-by: Laurent Pinchart
---
drivers/gpu/drm/rcar-du/rcar_du_crtc.c| 42 +++
drivers/gpu/drm/rcar-du/rcar_du_crtc.h| 7 ++--
drivers/gpu/drm/rcar-du/rcar_du_drv.h | 1 +
drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 10 --
drivers/gpu/drm/rcar-du/rcar_du_group.c | 4 +--
drivers/gpu/drm/rcar-du/rcar_du_kms.c | 22
6 files changed, 47 insertions(+), 39 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 90dacab67be5..40b7f17159b0 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -22,6 +22,7 @@
#include "rcar_du_crtc.h"
#include "rcar_du_drv.h"
+#include "rcar_du_encoder.h"
#include "rcar_du_kms.h"
#include "rcar_du_plane.h"
#include "rcar_du_regs.h"
@@ -316,26 +317,6 @@ static void rcar_du_crtc_set_display_timing(struct
rcar_du_crtc *rcrtc)
rcar_du_crtc_write(rcrtc, DEWR, mode->hdisplay);
}
-void rcar_du_crtc_route_output(struct drm_crtc *crtc,
- enum rcar_du_output output)
-{
- struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
- struct rcar_du_device *rcdu = rcrtc->group->dev;
-
- /*
-* Store the route from the CRTC output to the DU output. The DU will be
-* configured when starting the CRTC.
-*/
- rcrtc->outputs |= BIT(output);
-
- /*
-* Store RGB routing to DPAD0, the hardware will be configured when
-* starting the CRTC.
-*/
- if (output == RCAR_DU_OUTPUT_DPAD0)
- rcdu->dpad0_source = rcrtc->index;
-}
-
static unsigned int plane_zpos(struct rcar_du_plane *plane)
{
return plane->plane.state->normalized_zpos;
@@ -655,6 +636,24 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
* CRTC Functions
*/
+static int rcar_du_crtc_atomic_check(struct drm_crtc *crtc,
+struct drm_crtc_state *state)
+{
+ struct rcar_du_crtc_state *rstate = to_rcar_crtc_state(state);
+ struct drm_encoder *encoder;
+
+ /* Store the routes from the CRTC output to the DU outputs. */
+ rstate->outputs = 0;
+
+ drm_for_each_encoder_mask(encoder, crtc->dev, state->encoder_mask) {
+ struct rcar_du_encoder *renc = to_rcar_encoder(encoder);
+
+ rstate->outputs |= BIT(renc->output);
+ }
+
+ return 0;
+}
+
static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
struct drm_crtc_state *old_state)
{
@@ -678,8 +677,6 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc
*crtc,
crtc->state->event = NULL;
}
spin_unlock_irq(&crtc->dev->event_lock);
-
- rcrtc->outputs = 0;
}
static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc,
@@ -755,6 +752,7 @@ enum drm_mode_status rcar_du_crtc_mode_valid(struct
drm_crtc *crtc,
}
static const struct drm_crtc_helper_funcs crtc_helper_funcs = {
+ .atomic_check = rcar_du_crtc_atomic_check,
.atomic_begin = rcar_du_crtc_atomic_begin,
.atomic_flush = rcar_du_crtc_atomic_flush,
.atomic_enable = rcar_du_crtc_atomic_enable,
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
index 59ac6e7d22c9..ec47f164e69b 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
@@ -37,7 +37,6 @@ struct rcar_du_vsp;
* @vblank_lock: protects vblank_wait and vblank_count
* @vblank_wait: wait queue used to signal vertical blanking
* @vblank_count: number of vertical blanking interrupts to wait for
- * @outputs: bitmask of the outputs (enum rcar_du_output) driven by this CRTC
* @group: CRTC group this CRTC belongs to
* @vsp: VSP feeding video to this CRTC
* @vsp_pipe: index of the VSP pipeline feeding video to this CRTC
@@ -61,8 +60,6 @@ struct rcar_du_crtc {
wait_queue_head_t vblank_wait;
unsigned int vblank_count;
- unsigned int outputs;
-
struct rcar_du_group *group;
struct rcar_du_vsp *vsp;
unsigned int vsp_pipe;
@@ -77,1
[PATCH 0/5] R-Car DU: Fix DPAD output routing on D3 and E3
Hello, DU channels are routed to DPAD outputs in an SoC-dependent way. The routing can be fixed (e.g. DU3 to DPAD0 on H3) or configurable (e.g. DU0 or DU1 to DPAD0 on D3/E3). The hardware offers no option to disconnect DPAD outputs, which are thus always driven by a DU channel. On SoCs that have less DU channels than DU outputs, such as D3 and E3, the DPAD output is always driven when all channels are in used by other outputs (such as the internal LVDS and HDMI encoders). This creates an unwanted clone on the DPAD output. However, the parallel output of the DU channels routed to DPAD can be set to fixed levels in the DU channels themselves through the DOFLR group register. This patch series uses this feature to fix the problem and get rid of unwanted clones. Patch 1/5 is a small cleanup not strictly required by the series, but included as it was developed at the same time. Patch 2/5 moves output routing information from the CRTC structure to the CRTC state, in order to make the information available early enough for output routing configuration. Patch 3/5 then fixes the DPAD output routing issue using the DOFLR register. Patches 4/5 and 5/5 add backlight and panel support to the Draak board DT, in order to test the series. Only patch 4/5 should be upstreamed at this time as 5/5 should be rewritten using DT overlays. It can be merged separately from the rest of the series as code and DT are not dependent. Laurent Pinchart (5): drm: rcar-du: Replace EXT_CTRL_REGS feature flag with generation check drm: rcar-du: Move CRTC outputs bitmask to private CRTC state drm: rcar-du: Disable unused DPAD outputs arm64: dts: renesas: r8a77995: draak: Add backlight [HACK] arm64: dts: r8a77995: draak: Add panel to DT .../arm64/boot/dts/renesas/r8a77995-draak.dts | 31 drivers/gpu/drm/rcar-du/rcar_du_crtc.c| 42 drivers/gpu/drm/rcar-du/rcar_du_crtc.h| 7 +-- drivers/gpu/drm/rcar-du/rcar_du_drv.c | 14 +- drivers/gpu/drm/rcar-du/rcar_du_drv.h | 8 +-- drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 10 drivers/gpu/drm/rcar-du/rcar_du_group.c | 50 +-- drivers/gpu/drm/rcar-du/rcar_du_kms.c | 22 8 files changed, 126 insertions(+), 58 deletions(-) -- Regards, Laurent Pinchart
[PATCH 5/5] [HACK] arm64: dts: r8a77995: draak: Add panel to DT
Signed-off-by: Laurent Pinchart
---
arch/arm64/boot/dts/renesas/r8a77995-draak.dts | 11 +++
1 file changed, 11 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
index cd067319e6f3..140519e39f06 100644
--- a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
@@ -487,3 +487,14 @@
};
};
};
+
+#define lvds_connector lvds1_out
+#include "../../../../arm/boot/dts/r8a77xx-aa104xd12-panel.dtsi"
+
+&lvds1 {
+ status = "okay";
+};
+
+&{/panel/} {
+ backlight = <&backlight>;
+};
--
Regards,
Laurent Pinchart
[PATCH 3/5] drm: rcar-du: Disable unused DPAD outputs
DU channels are routed to DPAD outputs in an SoC-dependent way. The
routing can be fixed (e.g. DU3 to DPAD0 on H3) or configurable (e.g. DU0
or DU1 to DPAD0 on D3/E3). The hardware offers no option to disconnect
DPAD outputs, which are thus always driven by a DU channel.
On SoCs that have less DU channels than DU outputs, such as D3 and E3,
the DPAD output is always driven when all channels are in used by other
outputs (such as the internal LVDS and HDMI encoders). This creates an
unwanted clone on the DPAD output.
However, the parallel output of the DU channels routed to DPAD can be
set to fixed levels in the DU channels themselves through the DOFLR
group register. Use this to turn the DPAD on or off by driving fixed
signals at the output of any DU channel not routed to a DPAD output.
This doesn't affect the DU output signals going to other outputs.
Signed-off-by: Laurent Pinchart
---
drivers/gpu/drm/rcar-du/rcar_du_group.c | 42 +
1 file changed, 42 insertions(+)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c
b/drivers/gpu/drm/rcar-du/rcar_du_group.c
index 7e440f61977f..5aaf41b7a2ca 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
@@ -287,6 +287,46 @@ int rcar_du_set_dpad0_vsp1_routing(struct rcar_du_device
*rcdu)
return 0;
}
+static void rcar_du_group_set_output_levels(struct rcar_du_group *rgrp)
+{
+ static const u32 doflr_values[2] = {
+ DOFLR_HSYCFL0 | DOFLR_VSYCFL0 | DOFLR_ODDFL0 |
+ DOFLR_DISPFL0 | DOFLR_CDEFL0 | DOFLR_RGBFL0,
+ DOFLR_HSYCFL1 | DOFLR_VSYCFL1 | DOFLR_ODDFL1 |
+ DOFLR_DISPFL1 | DOFLR_CDEFL1 | DOFLR_RGBFL1,
+ };
+ static const u32 dpad_mask = BIT(RCAR_DU_OUTPUT_DPAD1)
+ | BIT(RCAR_DU_OUTPUT_DPAD0);
+ struct rcar_du_device *rcdu = rgrp->dev;
+ u32 doflr = DOFLR_CODE;
+ unsigned int i;
+
+ if (rcdu->info->gen < 2)
+ return;
+
+ /*
+* The DPAD outputs can't be controlled directly. However, the parallel
+* output of the DU channels routed to DPAD can be set to fixed levels
+* through the DOFLR group register. Use this to turn the DPAD on or off
+* by driving fixed signals at the output of any DU channel not routed
+* to a DPAD output. This doesn't affect the DU output signals going to
+* other outputs, such as the internal LVDS and HDMI encoders.
+*/
+
+ for (i = 0; i < rgrp->num_crtcs; ++i) {
+ struct rcar_du_crtc_state *rstate;
+ struct rcar_du_crtc *rcrtc;
+
+ rcrtc = &rcdu->crtcs[rgrp->index * 2 + i];
+ rstate = to_rcar_crtc_state(rcrtc->crtc.state);
+
+ if (!(rstate->outputs & dpad_mask))
+ doflr |= doflr_values[i];
+ }
+
+ rcar_du_group_write(rgrp, DOFLR, doflr);
+}
+
int rcar_du_group_set_routing(struct rcar_du_group *rgrp)
{
struct rcar_du_device *rcdu = rgrp->dev;
@@ -306,5 +346,7 @@ int rcar_du_group_set_routing(struct rcar_du_group *rgrp)
rcar_du_group_write(rgrp, DORCR, dorcr);
+ rcar_du_group_set_output_levels(rgrp);
+
return rcar_du_set_dpad0_vsp1_routing(rgrp->dev);
}
--
Regards,
Laurent Pinchart
[PATCH 1/5] drm: rcar-du: Replace EXT_CTRL_REGS feature flag with generation check
The RCAR_DU_FEATURE_EXT_CTRL_REGS feature flag is missing for H1 only,
which is a first generation device, not a second generation device as
reported in the device information table. Fix the H1 generation and use
generation checks to replace the feature flag.
Signed-off-by: Laurent Pinchart
---
drivers/gpu/drm/rcar-du/rcar_du_drv.c | 14 +-
drivers/gpu/drm/rcar-du/rcar_du_drv.h | 7 +++
drivers/gpu/drm/rcar-du/rcar_du_group.c | 4 ++--
3 files changed, 6 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index 94f055186b95..814c1099dacb 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -35,7 +35,6 @@
static const struct rcar_du_device_info rzg1_du_r8a7743_info = {
.gen = 2,
.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
- | RCAR_DU_FEATURE_EXT_CTRL_REGS
| RCAR_DU_FEATURE_INTERLACED
| RCAR_DU_FEATURE_TVM_SYNC,
.channels_mask = BIT(1) | BIT(0),
@@ -58,7 +57,6 @@ static const struct rcar_du_device_info rzg1_du_r8a7743_info
= {
static const struct rcar_du_device_info rzg1_du_r8a7745_info = {
.gen = 2,
.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
- | RCAR_DU_FEATURE_EXT_CTRL_REGS
| RCAR_DU_FEATURE_INTERLACED
| RCAR_DU_FEATURE_TVM_SYNC,
.channels_mask = BIT(1) | BIT(0),
@@ -80,7 +78,6 @@ static const struct rcar_du_device_info rzg1_du_r8a7745_info
= {
static const struct rcar_du_device_info rzg1_du_r8a77470_info = {
.gen = 2,
.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
- | RCAR_DU_FEATURE_EXT_CTRL_REGS
| RCAR_DU_FEATURE_INTERLACED
| RCAR_DU_FEATURE_TVM_SYNC,
.channels_mask = BIT(1) | BIT(0),
@@ -105,7 +102,7 @@ static const struct rcar_du_device_info
rzg1_du_r8a77470_info = {
};
static const struct rcar_du_device_info rcar_du_r8a7779_info = {
- .gen = 2,
+ .gen = 1,
.features = RCAR_DU_FEATURE_INTERLACED
| RCAR_DU_FEATURE_TVM_SYNC,
.channels_mask = BIT(1) | BIT(0),
@@ -128,7 +125,6 @@ static const struct rcar_du_device_info
rcar_du_r8a7779_info = {
static const struct rcar_du_device_info rcar_du_r8a7790_info = {
.gen = 2,
.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
- | RCAR_DU_FEATURE_EXT_CTRL_REGS
| RCAR_DU_FEATURE_INTERLACED
| RCAR_DU_FEATURE_TVM_SYNC,
.quirks = RCAR_DU_QUIRK_ALIGN_128B,
@@ -158,7 +154,6 @@ static const struct rcar_du_device_info
rcar_du_r8a7790_info = {
static const struct rcar_du_device_info rcar_du_r8a7791_info = {
.gen = 2,
.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
- | RCAR_DU_FEATURE_EXT_CTRL_REGS
| RCAR_DU_FEATURE_INTERLACED
| RCAR_DU_FEATURE_TVM_SYNC,
.channels_mask = BIT(1) | BIT(0),
@@ -182,7 +177,6 @@ static const struct rcar_du_device_info
rcar_du_r8a7791_info = {
static const struct rcar_du_device_info rcar_du_r8a7792_info = {
.gen = 2,
.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
- | RCAR_DU_FEATURE_EXT_CTRL_REGS
| RCAR_DU_FEATURE_INTERLACED
| RCAR_DU_FEATURE_TVM_SYNC,
.channels_mask = BIT(1) | BIT(0),
@@ -202,7 +196,6 @@ static const struct rcar_du_device_info
rcar_du_r8a7792_info = {
static const struct rcar_du_device_info rcar_du_r8a7794_info = {
.gen = 2,
.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
- | RCAR_DU_FEATURE_EXT_CTRL_REGS
| RCAR_DU_FEATURE_INTERLACED
| RCAR_DU_FEATURE_TVM_SYNC,
.channels_mask = BIT(1) | BIT(0),
@@ -225,7 +218,6 @@ static const struct rcar_du_device_info
rcar_du_r8a7794_info = {
static const struct rcar_du_device_info rcar_du_r8a7795_info = {
.gen = 3,
.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
- | RCAR_DU_FEATURE_EXT_CTRL_REGS
| RCAR_DU_FEATURE_VSP1_SOURCE
| RCAR_DU_FEATURE_INTERLACED
| RCAR_DU_FEATURE_TVM_SYNC,
@@ -259,7 +251,6 @@ static const struct rcar_du_device_info
rcar_du_r8a7795_info = {
static const struct rcar_du_device_info rcar_du_r8a7796_info = {
.gen = 3,
.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
- | RCAR_DU_FEATURE_EXT_CTRL_REGS
| RCAR_DU_FEATURE_VSP1_SOURCE
| RCAR_DU_FEATURE_INTERLACED
| RCAR_DU_FEATURE_TVM_SYNC,
@@ -289,7 +280,6 @@ static const struct rcar_du_device_info
rcar_du_r8a7796_info = {
static const struct rcar_du_device_info rcar_du_r8a77965_info = {
.gen = 3,
.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
- | RCAR_DU_FEATURE_EXT_CTRL_REGS
| RCAR_DU_FEATURE_VSP1_SOURCE
[PATCH 4/5] arm64: dts: renesas: r8a77995: draak: Add backlight
Add the backlight device for the LVDS1 output, in preparation for panel
support.
Signed-off-by: Laurent Pinchart
---
.../arm64/boot/dts/renesas/r8a77995-draak.dts | 20 +++
1 file changed, 20 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
index 2405eaad0296..cd067319e6f3 100644
--- a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
@@ -24,6 +24,17 @@
stdout-path = "serial0:115200n8";
};
+ backlight: backlight {
+ compatible = "pwm-backlight";
+ pwms = <&pwm1 0 5>;
+
+ brightness-levels = <256 128 64 16 8 4 0>;
+ default-brightness-level = <6>;
+
+ power-supply = <®_12p0v>;
+ enable-gpios = <&gpio4 0 GPIO_ACTIVE_HIGH>;
+ };
+
composite-in {
compatible = "composite-video-connector";
@@ -104,6 +115,15 @@
regulator-always-on;
};
+ reg_12p0v: regulator1 {
+ compatible = "regulator-fixed";
+ regulator-name = "D12.0V";
+ regulator-min-microvolt = <1200>;
+ regulator-max-microvolt = <1200>;
+ regulator-boot-on;
+ regulator-always-on;
+ };
+
vga {
compatible = "vga-connector";
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: rcar-du: dw-hdmi: Reject modes with a too high clock frequency
Hi Kieran,
On Friday, 23 November 2018 17:30:43 EET Kieran Bingham wrote:
> On 23/11/2018 14:47, Laurent Pinchart wrote:
> > On Friday, 23 November 2018 16:43:28 EET Kieran Bingham wrote:
> >> On 23/11/2018 14:34, Laurent Pinchart wrote:
> >>> Implement a .mode_valid() handler in the R-Car glue layer to reject
> >>> modes with an unsupported clock frequency.
> >>>
> >>> Signed-off-by: Laurent Pinchart
> >>>
> >>> ---
> >>>
> >>> drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c | 11 +++
> >>> 1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
> >>> b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c index
> >>> 75490a3e0a2a..8a603235f22d
> >>> 100644
> >>> --- a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
> >>> +++ b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
> >>> @@ -35,6 +35,16 @@ static const struct rcar_hdmi_phy_params
> >>> rcar_hdmi_phy_params[] = {
> >>> { ~0UL, 0x, 0x, 0x },
> >>> };
> >>>
> >>> +static enum drm_mode_status
> >>> +rcar_hdmi_mode_valid(struct drm_connector *connector,
> >>> + const struct drm_display_mode *mode)
> >>> +{
> >>> + if (mode->clock > 297000)
> >>
> >> Is 29700 constant? Can it be determined from any other location or is it
> >> just a magically known platform value?
> >
> > It's the last entry of the rcar_hdmi_phy_params table above. I considered
> > writing it
> >
> > if (mode->clock >
> >
> > rcar_hdmi_phy_params[ARRAY_SIZE(rcar_hdmi_phy_params)-2].mpixelclock)
> >
> > but found it a but hard to parse. Do you think it would be better ?
>
> Well - for readability - no,
>
> But for accuracy - yes:
>
> 29700 != 297000
>
> Unless the /1000 is intentional?
I would have had to write / 1000 indeed :-) mode->clock is expressed in kHz.
> How about keep the (corrected?) constant value, but add a comment
> referencing it's extraction.
Good idea, I'll do so.
> I don't think the coded table extraction helps here. Especially as it
> necessitates indexing against ARRAY_SIZE - 2.
>
> >>> + return MODE_CLOCK_HIGH;
> >>> +
> >>> + return MODE_OK;
> >>> +}
> >>> +
> >>> static int rcar_hdmi_phy_configure(struct dw_hdmi *hdmi,
> >>> const struct dw_hdmi_plat_data *pdata,
> >>> unsigned long mpixelclock)
> >>> @@ -59,6 +69,7 @@ static int rcar_hdmi_phy_configure(struct dw_hdmi
> >>> *hdmi,
> >>> }
> >>>
> >>> static const struct dw_hdmi_plat_data rcar_dw_hdmi_plat_data = {
> >>> + .mode_valid = rcar_hdmi_mode_valid,
> >>> .configure_phy = rcar_hdmi_phy_configure,
> >>> };
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: rcar-du: dw-hdmi: Reject modes with a too high clock frequency
Hi Kieran,
On Friday, 23 November 2018 16:43:28 EET Kieran Bingham wrote:
> On 23/11/2018 14:34, Laurent Pinchart wrote:
> > Implement a .mode_valid() handler in the R-Car glue layer to reject
> > modes with an unsupported clock frequency.
> >
> > Signed-off-by: Laurent Pinchart
> >
> > ---
> >
> > drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c | 11 +++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
> > b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c index 75490a3e0a2a..8a603235f22d
> > 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
> > @@ -35,6 +35,16 @@ static const struct rcar_hdmi_phy_params
> > rcar_hdmi_phy_params[] = {
> > { ~0UL, 0x, 0x, 0x },
> >
> > };
> >
> > +static enum drm_mode_status
> > +rcar_hdmi_mode_valid(struct drm_connector *connector,
> > +const struct drm_display_mode *mode)
> > +{
> > + if (mode->clock > 297000)
>
> Is 29700 constant? Can it be determined from any other location or is it
> just a magically known platform value?
It's the last entry of the rcar_hdmi_phy_params table above. I considered
writing it
if (mode->clock >
rcar_hdmi_phy_params[ARRAY_SIZE(rcar_hdmi_phy_params)-2].mpixelclock)
but found it a but hard to parse. Do you think it would be better ?
> > + return MODE_CLOCK_HIGH;
> > +
> > + return MODE_OK;
> > +}
> > +
> >
> > static int rcar_hdmi_phy_configure(struct dw_hdmi *hdmi,
> >const struct dw_hdmi_plat_data *pdata,
> >unsigned long mpixelclock)
> > @@ -59,6 +69,7 @@ static int rcar_hdmi_phy_configure(struct dw_hdmi *hdmi,
> > }
> >
> > static const struct dw_hdmi_plat_data rcar_dw_hdmi_plat_data = {
> > + .mode_valid = rcar_hdmi_mode_valid,
> > .configure_phy = rcar_hdmi_phy_configure,
> > };
--
Regards,
Laurent Pinchart
Re: [PATCH v3 4/4] i2c: adv748x: configure number of lanes used for TXA CSI-2 transmitter
them.
>
> And actually, I don't like much the comments for registers pll_en and
> dphy_pdn registers, and they might be improved, since you're rewriting
> this sequence anyhow.
>
> I had a patch pending, before I realized you could change this in your
> next v4. In case you want to have a look:
> https://paste.debian.net/1052965/
I would prefer fixes to be made on top of this patch, to separate the
refactoring from the functional changes as much as possible.
> > +
> > + /* ADI Required Write */
> > + adv748x_write_check(state, page, 0xc1, 0x3b, &ret);
> > +
> > + return ret;
> > +}
> >
> > int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
> > {
> > - struct adv748x_state *state = tx->state;
> > - const struct adv748x_reg_value *reglist;
> > int val;
> >
> > if (!is_tx_enabled(tx))
> > @@ -321,14 +329,7 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool
> > on)
> > WARN_ONCE((on && val & ADV748X_CSI_FS_AS_LS_UNKNOWN),
> > "Enabling with unknown bit set");
> >
> > - if (on)
> > - reglist = is_txa(tx) ? adv748x_power_up_txa_4lane :
> > - adv748x_power_up_txb_1lane;
> > - else
> > - reglist = is_txa(tx) ? adv748x_power_down_txa_4lane :
> > - adv748x_power_down_txb_1lane;
> > -
> > - return adv748x_write_regs(state, reglist);
> > + return on ? adv748x_power_up_tx(tx) : adv748x_power_down_tx(tx);
> > }
> >
> > /* --
--
Regards,
Laurent Pinchart
[PATCH] drm: rcar-du: dw-hdmi: Reject modes with a too high clock frequency
Implement a .mode_valid() handler in the R-Car glue layer to reject
modes with an unsupported clock frequency.
Signed-off-by: Laurent Pinchart
---
drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
index 75490a3e0a2a..8a603235f22d 100644
--- a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
+++ b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
@@ -35,6 +35,16 @@ static const struct rcar_hdmi_phy_params
rcar_hdmi_phy_params[] = {
{ ~0UL, 0x, 0x, 0x },
};
+static enum drm_mode_status
+rcar_hdmi_mode_valid(struct drm_connector *connector,
+const struct drm_display_mode *mode)
+{
+ if (mode->clock > 297000)
+ return MODE_CLOCK_HIGH;
+
+ return MODE_OK;
+}
+
static int rcar_hdmi_phy_configure(struct dw_hdmi *hdmi,
const struct dw_hdmi_plat_data *pdata,
unsigned long mpixelclock)
@@ -59,6 +69,7 @@ static int rcar_hdmi_phy_configure(struct dw_hdmi *hdmi,
}
static const struct dw_hdmi_plat_data rcar_dw_hdmi_plat_data = {
+ .mode_valid = rcar_hdmi_mode_valid,
.configure_phy = rcar_hdmi_phy_configure,
};
--
Regards,
Laurent Pinchart
Re: [PATCH 4/5] drm: rcar-du: Add R8A7744 support
Hi Fabrizio,
On Thursday, 22 November 2018 17:59:32 EET Fabrizio Castro wrote:
> On 15 October 2018 23:25 Laurent Pinchart wrote:
> > On Friday, 21 September 2018 21:08:30 EEST Fabrizio Castro wrote:
> >> From: Biju Das
> >>
> >> Add support for the R8A7744 DU (which is very similar to the R8A7743
> >> DU);
> >> it has 1 DPAD (RGB) output and 1 LVDS output.
> >>
> >> Signed-off-by: Biju Das
> >> Reviewed-by: Fabrizio Castro
> >> ---
> >>
> >> drivers/gpu/drm/rcar-du/rcar_du_drv.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index c07d3f1..2c3d0e5 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >> @@ -321,6 +321,7 @@ static const struct rcar_du_device_info
> >> rcar_du_r8a77970_info = {
> >> static const struct of_device_id rcar_du_of_table[] = {
> >> { .compatible = "renesas,du-r8a7743", .data = &rzg1_du_r8a7743_info },
> >> +{ .compatible = "renesas,du-r8a7744", .data = &rzg1_du_r8a7743_info },
> >> { .compatible = "renesas,du-r8a7745", .data = &rzg1_du_r8a7745_info },
> >> { .compatible = "renesas,du-r8a77470", .data = &rzg1_du_r8a77470_info
> >> },
> >> { .compatible = "renesas,du-r8a7779", .data = &rcar_du_r8a7779_info },
> >
> > This looks good to me. I would also apply this change:
> >
> > @@ -41,7 +41,7 @@ static const struct rcar_du_device_info
> > rzg1_du_r8a7743_info = {
> > .channels_mask = BIT(1) | BIT(0),
> > .routes = {
> > /*
> > -* R8A7743 has one RGB output and one LVDS output
> > +* R8A774[34] has one RGB output and one LVDS output
> > */
> > [RCAR_DU_OUTPUT_DPAD0] = {
> > .possible_crtcs = BIT(1) | BIT(0),
> >
> > With this,
> >
> > Reviewed-by: Laurent Pinchart
> >
> > There's no need to resubmit, I've applied the patch to my tree with the
> > above change.
>
> I was expecting to see this patch at least on linux-next by now but it looks
> like it's not in there, could you please double check what happened to it?
I've sent the R-Car DU v4.21 pull request today, it should end up in linux-
next in the near future.
--
Regards,
Laurent Pinchart
Re: [PATCH v2 3/5] drm: rcar-du: Add r8a77470 support
Hi Fabrizio,
On Thursday, 22 November 2018 18:03:44 EET Fabrizio Castro wrote:
> On 17 October 2018 07:52 Laurent Pinchart wrote:
> > On Tuesday, 16 October 2018 19:58:59 EEST Fabrizio Castro wrote:
> > > Add RZ/G1C (a.k.a. r8a77470) support to the R-Car DU driver.
> >>
> >> Signed-off-by: Fabrizio Castro
> >> Reviewed-by: Laurent Pinchart
> >>
> >> ---
> >> v1->v2:
> >> * Added flags RCAR_DU_FEATURE_INTERLACED and RCAR_DU_FEATURE_TVM_SYNC
> >> * Reworked comment
> >
> > This looks all good, applied to my tree.
>
> It looks like I can't find the patch, which tree is it?
The patch can be found in git://linuxtv.org/pinchartl/media.git drm/du/next.
> >> drivers/gpu/drm/rcar-du/rcar_du_drv.c | 28
> >> 1 file changed, 28 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 084f58d..d8a02c4 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> >> @@ -77,6 +77,33 @@ static const struct rcar_du_device_info
> >> rzg1_du_r8a7745_info = { },
> >> };
> >>
> >> +static const struct rcar_du_device_info rzg1_du_r8a77470_info = {
> >> +.gen = 2,
> >> +.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
> >> + | RCAR_DU_FEATURE_EXT_CTRL_REGS
> >> + | RCAR_DU_FEATURE_INTERLACED
> >> + | RCAR_DU_FEATURE_TVM_SYNC,
> >> +.channels_mask = BIT(1) | BIT(0),
> >> +.routes = {
> >> +/*
> >> + * R8A77470 has two RGB outputs, one LVDS output, and
> >> + * one (currently unsupported) analog video output
> >> + */
> >> +[RCAR_DU_OUTPUT_DPAD0] = {
> >> +.possible_crtcs = BIT(0),
> >> +.port = 0,
> >> +},
> >> +[RCAR_DU_OUTPUT_DPAD1] = {
> >> +.possible_crtcs = BIT(1),
> >> +.port = 1,
> >> +},
> >> +[RCAR_DU_OUTPUT_LVDS0] = {
> >> +.possible_crtcs = BIT(0) | BIT(1),
> >> +.port = 2,
> >> +},
> >> +},
> >> +};
> >> +
> >> static const struct rcar_du_device_info rcar_du_r8a7779_info = {
> >> .gen = 2,
> >> .features = RCAR_DU_FEATURE_INTERLACED
> >> @@ -342,6 +369,7 @@ static const struct rcar_du_device_info
> >> rcar_du_r8a7799x_info = { static const struct of_device_id
> >> rcar_du_of_table[] = {
> >> { .compatible = "renesas,du-r8a7743", .data = &rzg1_du_r8a7743_info },
> >> { .compatible = "renesas,du-r8a7745", .data = &rzg1_du_r8a7745_info },
> >> +{ .compatible = "renesas,du-r8a77470", .data = &rzg1_du_r8a77470_info
> >> },
> >> { .compatible = "renesas,du-r8a7779", .data = &rcar_du_r8a7779_info },
> >> { .compatible = "renesas,du-r8a7790", .data = &rcar_du_r8a7790_info },
> >> { .compatible = "renesas,du-r8a7791", .data = &rcar_du_r8a7791_info },
--
Regards,
Laurent Pinchart
[PATCH] drm: rcar-du: Fix DU3 start/stop on M3-N
Group start/stop is controlled by the DRES and DEN bits of DSYSR0 for
the first group and DSYSR2 for the second group. On most DU instances,
this maps to the first CRTC of the group. On M3-N, however, DU2 doesn't
exist, but DSYSR2 does. There is no CRTC object there that maps to the
correct DSYSR register.
Commit 9144adc5e5a9 ("drm: rcar-du: Cache DSYSR value to ensure known
initial value") switched group start/stop from using group read/write
access to DSYSR to a CRTC-based API to cache the DSYSR value. While
doing so, it introduced a regression on M3-N by accessing DSYSR3 instead
of DSYSR2 to start/stop the second group.
To fix this, access the DSYSR register directly through group read/write
if the SoC is missing the first DU channel of the group. Keep using the
rcar_du_crtc_dsysr_clr_set() function otherwise, to retain the DSYSR
caching feature.
Fixes: 9144adc5e5a9 ("drm: rcar-du: Cache DSYSR value to ensure known initial
value")
Reported-by: Hoan Nguyen An
Signed-off-by: Laurent Pinchart
---
drivers/gpu/drm/rcar-du/rcar_du_group.c | 21 ++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c
b/drivers/gpu/drm/rcar-du/rcar_du_group.c
index d85f0a1c1581..cebf313c6e1f 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
@@ -202,10 +202,25 @@ void rcar_du_group_put(struct rcar_du_group *rgrp)
static void __rcar_du_group_start_stop(struct rcar_du_group *rgrp, bool start)
{
- struct rcar_du_crtc *rcrtc = &rgrp->dev->crtcs[rgrp->index * 2];
+ struct rcar_du_device *rcdu = rgrp->dev;
+
+ /*
+* Group start/stop is controlled by the DRES and DEN bits of DSYSR0
+* for the first group and DSYSR2 for the second group. On most DU
+* instances, this maps to the first CRTC of the group, and we can just
+* use rcar_du_crtc_dsysr_clr_set() to access the correct DSYSR. On
+* M3-N, however, DU2 doesn't exist, but DSYSR2 does. We thus need to
+* access the register directly using group read/write.
+*/
+ if (rcdu->info->channels_mask & BIT(rgrp->index * 2)) {
+ struct rcar_du_crtc *rcrtc = &rgrp->dev->crtcs[rgrp->index * 2];
- rcar_du_crtc_dsysr_clr_set(rcrtc, DSYSR_DRES | DSYSR_DEN,
- start ? DSYSR_DEN : DSYSR_DRES);
+ rcar_du_crtc_dsysr_clr_set(rcrtc, DSYSR_DRES | DSYSR_DEN,
+ start ? DSYSR_DEN : DSYSR_DRES);
+ } else {
+ rcar_du_group_write(rgrp, DSYSR,
+ start ? DSYSR_DEN : DSYSR_DRES);
+ }
}
void rcar_du_group_start_stop(struct rcar_du_group *rgrp, bool start)
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: rcar-du: Re-update the DSYSR register value for start/stop
Hello Hoan, On Tuesday, 23 October 2018 04:01:19 EET Hoan wrote: > On 2018/10/22 20:23, Laurent Pinchart wrote: > > On Monday, 22 October 2018 09:30:54 EEST Nguyen An Hoan wrote: > >> From: Hoan Nguyen An > >> > >> From previous commit 0521ccb "drm: rcar-du: Cache DSYSR value to ensure > >> > >> known initial value" > > > > What exact commit are you referring to ? The mainline commit that has this > > subject is 9144adc5e5a99577bce0d4ee2ca3615f53b9d296. > > Seems I have cited the wrong Commit-ID、it is > > 9144adc5e5a99577bce0d4ee2ca3615f53b9d296 > drm: rcar-du: Cache DSYSR value to ensure known initial value > > >> We only need to update DSYSR0, DSYSR2 for start/stop. > >> So using rgrp-> mmio_offset is enough, the change back from rcar_du_crtc > >> ->rcar_du_group -> rcar_du_crtc leading to mmio addresses for DSYSR may > >> be different. > > > > Is this fixing an actual problem ? If you look at the code, the line > > > > struct rcar_du_crtc *rcrtc = &rgrp->dev->crtcs[rgrp->index * 2]; > > > > makes sure that we select DU0 or DU2 only, so the register write > > > > rcar_du_crtc_dsysr_clr_set(rcrtc, DSYSR_DRES | DSYSR_DEN, > >start ? DSYSR_DEN : DSYSR_DRES); > > > > should only access DSYSR0 and DSYSR2. > > This seems I think to fix the rcar-du problem with M3N-r8a77965, > > With M3N-R8a77965 we have DU0, DU1, DU3 > So, when Laurent-san divide objetcs into rcar_du_group, rcar_du_crtc. > > DU0, DU1 -> du_group[0] rgrp-> mmio_offset = DU0_REG_OFFSET > DU3->du_group[1] and rgrp-> mmio_offset = DU2_REG_OFFSET, but > rcrtc->mmio_offset=DU3_REG_OFFSET (with M3N) > > M3N-R8a77965 not have DU2, So after the command: > > struct rcar_du_crtc *rcrtc = &rgrp->dev->crtcs[rgrp->index * 2]; > > So in fact, with M3N we are updating DSYSR3 (In this my TC, this > reference to start/stop DU3-RGB) > > This will not affect H3, since the H3 lines always have enough DU0, > DU1,DU2,DU3. You're absolutely right. I'm sorry for introducing the bug in the first place, and for failing to understand your patch. I would however prefer a different fix, as switching back to rcar_du_group_write() defeats the purpose of the "drm: rcar-du: Cache DSYSR value to ensure known initial value" patch. I will submit a patch and CC you. -- Regards, Laurent Pinchart
Re: [PATCH] drm: rcar-du: fix probe error when DRM_RCAR_DW_HDMI disabled
Hello Hoan,
Thank you for the patch.
On Friday, 21 September 2018 14:49:03 EET Nguyen An Hoan wrote:
> From: Hoan Nguyen An
>
> Skip return EPROBE_DEFER when DRM_RCAR_DW_HDMI is disabled in case HDMI
> initialize. At this time, the rcar-du driver not be able to successfully
> initialize if disable DRM_RCAR_DW_HDMI (rcar_du_probe return error),
> so can not use other features such as RGB Analog, this patch to fix.
>
> Signed-off-by: Hoan Nguyen An
> ---
> drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c index f9c933d..4dbc508 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> @@ -66,8 +66,15 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
> /* Locate the DRM bridge from the encoder DT node. */
> bridge = of_drm_find_bridge(enc_node);
> if (!bridge) {
> +#if IS_ENABLED(CONFIG_DRM_RCAR_DW_HDMI)
> ret = -EPROBE_DEFER;
> goto done;
> +#else
> + if (output == RCAR_DU_OUTPUT_HDMI0 ||
> + output == RCAR_DU_OUTPUT_HDMI1)
> + ret = 0;
> + goto done;
> +#endif
The dw-hdmi driver could be compiled out of tree, I don't think is the right
fix. If the HDMI output should be disabled on a particular system, the
corresponding DT node should be marked as disable, and no change to the DU
driver is needed. Otherwise the necessary drivers should be present.
In the long term we should implement support for registration of additional
encoders at runtime, but that's way down the road.
> }
>
> ret = drm_encoder_init(rcdu->ddev, encoder, &encoder_funcs,
--
Regards,
Laurent Pinchart
[PATCH] media: vsp1: Fix LIF buffer thresholds
Commit de2bc45c84f7 ("media: vsp1: Update LIF buffer thresholds")
updated the LIF buffer thresholds based on the VSP version, but used the
wrong model mask. This resulted in all VSP instances to be treated as a
Gen3 VSPD, breaking operation on all Gen2 platforms as well as on
H3 ES2.0, M3-N, V3M and V3H. Fix it.
Fixes: de2bc45c84f7 ("media: vsp1: Update LIF buffer thresholds")
Reported-by: Colin Ian King
Signed-off-by: Laurent Pinchart
---
drivers/media/platform/vsp1/vsp1_lif.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/vsp1/vsp1_lif.c
b/drivers/media/platform/vsp1/vsp1_lif.c
index 0b18f0bd7419..8b0a26335d70 100644
--- a/drivers/media/platform/vsp1/vsp1_lif.c
+++ b/drivers/media/platform/vsp1/vsp1_lif.c
@@ -95,7 +95,7 @@ static void lif_configure_stream(struct vsp1_entity *entity,
format = vsp1_entity_get_pad_format(&lif->entity, lif->entity.config,
LIF_PAD_SOURCE);
- switch (entity->vsp1->version & VI6_IP_VERSION_SOC_MASK) {
+ switch (entity->vsp1->version & VI6_IP_VERSION_MODEL_MASK) {
case VI6_IP_VERSION_MODEL_VSPD_GEN2:
case VI6_IP_VERSION_MODEL_VSPD_V2H:
hbth = 1536;
--
Regards,
Laurent Pinchart
[PATCH v2] drm: rcar-du: Reject modes that fail CRTC timing requirements
The hardware requires the HDSR and VDSR registers to be set to 1 or
higher. This translates to a minimum combined horizontal sync and back
porch of 20 pixels and a minimum vertical back porch of 3 lines. Reject
modes that fail those requirements.
Signed-off-by: Laurent Pinchart
---
Changes since v1:
- Don't use mode->crtc_v* as they are only set after calling .mode_valid()
- Tested the patch on real hardware (which would have prevented the
buggy v1...)
---
drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 12
1 file changed, 12 insertions(+)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 79021d7aa3ce..90dacab67be5 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -735,10 +735,22 @@ enum drm_mode_status rcar_du_crtc_mode_valid(struct
drm_crtc *crtc,
struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
struct rcar_du_device *rcdu = rcrtc->group->dev;
bool interlaced = mode->flags & DRM_MODE_FLAG_INTERLACE;
+ unsigned int vbp;
if (interlaced && !rcar_du_has(rcdu, RCAR_DU_FEATURE_INTERLACED))
return MODE_NO_INTERLACE;
+ /*
+* The hardware requires a minimum combined horizontal sync and back
+* porch of 20 pixels and a minimum vertical back porch of 3 lines.
+*/
+ if (mode->htotal - mode->hsync_start < 20)
+ return MODE_HBLANK_NARROW;
+
+ vbp = (mode->vtotal - mode->vsync_end) / (interlaced ? 2 : 1);
+ if (vbp < 3)
+ return MODE_VBLANK_NARROW;
+
return MODE_OK;
}
--
Regards,
Laurent Pinchart
Re: [PATCH 2/2] drm: rcar-du: Enable alpha property on primary planes
Hi Kieran,
On Thursday, 20 September 2018 14:22:38 EET Laurent Pinchart wrote:
> On Wednesday, 19 September 2018 18:56:59 EEST Kieran Bingham wrote:
> > If the alpha property is not added to a plane, a default value will be
> > used, which can result in a non-visible layer if the alpha is
> > initialised as 0.
> >
> > Provide an alpha blend property on all planes.
> >
> > Fixes: 161ad653d6c9 ("drm: rcar-du: Use __drm_atomic_helper_plane_reset
> > instead of copying the logic")
> >
> > Signed-off-by: Kieran Bingham
> > ---
> >
> > drivers/gpu/drm/rcar-du/rcar_du_plane.c | 7 ++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_plane.c index 9e07758a755c..72399a19d8a6
> > 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> > @@ -783,13 +783,18 @@ int rcar_du_planes_init(struct rcar_du_group *rgrp)
> >
> > drm_plane_helper_add(&plane->plane,
> >
> > &rcar_du_plane_helper_funcs);
> >
> > + /*
> > +* The alpha property needs to be initialised on all planes
> > +* to ensure the correct setting at the output.
> > +*/
> > + drm_plane_create_alpha_property(&plane->plane);
> > +
>
> As mentioned in the cover letter, both patches in this series fix the issue
> at hand. The first patch is more generic as it will fix it for all drivers,
> while this patch is specific to the R-Car DU driver. It however makes sense
> to merge it, as it adds alpha support to the primary plane, which can be
> useful.
>
> Once the first patch gets merged, the above comment won't be correct
> anymore. I wonder whether we shouldn't change the patch description and
> comment to focus on usage of the alpha property for primary planes, and not
> on the bug fix. What's your opinion ?
I've removed the comment and changed the commit message to
drm: rcar-du: Enable alpha property on primary planes
The hardware supports alpha on all planes, and using it on the primary
plane can be useful. Don't restrict the alpha property to overlay
planes.
With this,
Reviewed-by: Laurent Pinchart
and applied to my tree.
> > if (type == DRM_PLANE_TYPE_PRIMARY)
> > continue;
> >
> > drm_object_attach_property(&plane->plane.base,
> >rcdu->props.colorkey,
> >RCAR_DU_COLORKEY_NONE);
> >
> > - drm_plane_create_alpha_property(&plane->plane);
> > drm_plane_create_zpos_property(&plane->plane, 1, 1, 7);
> > }
--
Regards,
Laurent Pinchart
[PATCH] drm: rcar-du: Reject modes that fail CRTC timing requirements
The hardware requires the HDSR and VDSR registers to be set to 1 or higher. This translates to a minimum combined horizontal sync and back porch of 20 pixels and a minimum vertical back porch of 3 lines. Reject modes that fail those requirements. Signed-off-by: Laurent Pinchart --- drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 9 + 1 file changed, 9 insertions(+) diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 79021d7aa3ce..9d425894e000 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -739,6 +739,15 @@ enum drm_mode_status rcar_du_crtc_mode_valid(struct drm_crtc *crtc, if (interlaced && !rcar_du_has(rcdu, RCAR_DU_FEATURE_INTERLACED)) return MODE_NO_INTERLACE; + /* +* The hardware requires a minimum combined horizontal sync and back +* porch of 20 pixels and a minimum vertical back porch of 3 lines. +*/ + if (mode->htotal - mode->hsync_start < 20) + return MODE_HBLANK_NARROW; + if (mode->crtc_vtotal - mode->crtc_vsync_end < 3) + return MODE_VBLANK_NARROW; + return MODE_OK; } -- Regards, Laurent Pinchart
Re: HDMI doesn't work on ULCB board
Hi Morimoto-san, On Monday, 12 November 2018 05:09:13 EET Kuninori Morimoto wrote: > Hi Laurent > > >> I noticed Salvator can't boot if .config has CONFIG_COMMON_CLK_VC5. > >> This means, Salvator and ULCB can't use same binary so far for me > >> (= all modules are =y on .config). > >> I'm using previous attached .config + your patch + CONFIG_COMMON_CLK_VC5 > > (snip) > > > I'm afraid I can't reproduce this. I checked out > > renesas-drivers-2018-10-09- v4.19-rc7, applied the fix I've submitted, > > took your configuration, enabled CONFIG_COMMON_CLK_VC5, and my > > Salvator-XS H3 boots fine. > > I'm deeply sorry, it was my fault. > I'm using ES1.x board, but I used XS dtb I've done worse mistakes, and many times, so I can't blame you :-) I'm glad it works now. -- Regards, Laurent Pinchart
Re: HDMI doesn't work on ULCB board
Hi Morimoto-san, On Friday, 9 November 2018 04:23:36 EET Kuninori Morimoto wrote: > Hi Laurent > > >>> I tried to reproduce the problem, starting with drm-next which seems > >>> to work fine, moving to renesas-drivers-2018-10-09-v4.19-rc7 which > >>> also didn't exhibit any issue. I then used your kernel configuration, > >>> and got a WARN_ON \o/ > >> > >> Investigations revealed that you're missing the CONFIG_COMMON_CLK_VC5=y > >> option in your kernel configuration. It also revealed a bug in the > >> error handling code of the DU driver, for which I have just sent > >> "[PATCH] drm: rcar-du: Fix external clock error checks". > > > > Thanks !! > > I tried your posted patch, and it solved Oops issue, > > and CONFIG_COMMON_CLK_VC5 solved HDMI outputs !! > > Hmm... > > I noticed Salvator can't boot if .config has CONFIG_COMMON_CLK_VC5. > This means, Salvator and ULCB can't use same binary so far for me > (= all modules are =y on .config). > I'm using previous attached .config + your patch + CONFIG_COMMON_CLK_VC5 > > Kernel will stop here on Salvator > > - > > rcar-fcp fe9af000.fcp: ignoring dependency for device, assuming no driver > rcar-fcp fe9bf000.fcp: ignoring dependency for device, assuming no driver > rcar-fcp fea27000.fcp: ignoring dependency for device, assuming no driver > rcar-fcp fea2f000.fcp: ignoring dependency for device, assuming no driver > rcar-fcp fea37000.fcp: ignoring dependency for device, assuming no driver > [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). > [drm] No driver support for vblank timestamp query. > > ** stop here ** I'm afraid I can't reproduce this. I checked out renesas-drivers-2018-10-09- v4.19-rc7, applied the fix I've submitted, took your configuration, enabled CONFIG_COMMON_CLK_VC5, and my Salvator-XS H3 boots fine. -- Regards, Laurent Pinchart
Re: [PATCH v2] arm64: dts: renesas: r8a77990: Fix VIN endpoint numbering
Hi Simon,
On Tuesday, 6 November 2018 16:00:35 EET Simon Horman wrote:
> On Mon, Nov 05, 2018 at 02:12:43PM +0100, Jacopo Mondi wrote:
> > The VIN driver bindings dictates fixed numbering for VIN endpoints
> > connected to CSI-2 endpoints, even when a single endpoint exists.
> >
> > Without proper endpoint numbering the VIN driver fails to probe.
> >
> > Based on a patch in BSP from Koji Matsuoka
> >
> > Fixes: ec70407ae7d7 ("arm64: dts: renesas: r8a77990: Add VIN and CSI-2
> > device nodes") Signed-off-by: Koji Matsuoka
> >
> > Signed-off-by: Takeshi Kihara
> > Signed-off-by: Jacopo Mondi
> > Reviewed-by: Laurent Pinchart
>
> Thanks,
>
> This looks fine to me but I will wait to see if there are other reviews
> before applying.
>
> Reviewed-by: Simon Horman
I think you can go ahead and apply it.
--
Regards,
Laurent Pinchart
Re: HDMI doesn't work on ULCB board
Hi Morimoto-san, On Tuesday, 6 November 2018 16:30:35 EET Laurent Pinchart wrote: > On Thursday, 18 October 2018 05:12:14 EET Kuninori Morimoto wrote: > > Hi Vladimir, Laurent > > > > I'm testing ULCB board, and I noticed that HDMI doen't work on it. > > > > - Sometimes kernel panic happen (I lost log, but it said "get crc" > > > > something error), - Often, kernel doesn't boot with below log if I > > connected HDMI cable when booting > > > > - Monitor say "No signal" if I connected cable *after* booting > > > > I'm using Geert's renesas-drivers-2018-10-09-v4.19-rc7 branch. > > It seems salvator HDMI is working correctly on it. > > I attached my .config > > I tried to reproduce the problem, starting with drm-next which seems to work > fine, moving to renesas-drivers-2018-10-09-v4.19-rc7 which also didn't > exhibit any issue. I then used your kernel configuration, and got a WARN_ON > \o/ Investigations revealed that you're missing the CONFIG_COMMON_CLK_VC5=y option in your kernel configuration. It also revealed a bug in the error handling code of the DU driver, for which I have just sent "[PATCH] drm: rcar-du: Fix external clock error checks". -- Regards, Laurent Pinchart
[PATCH] drm: rcar-du: Fix external clock error checks
The rcar-du driver supports probe deferral for external clocks, but
implements it badly by checking the wrong pointer due to a bad copy and
paste. Fix it.
While at it, reject invalid clocks outright for DU channels that have a
display PLL, as the external clock is mandatory in that case. This
avoids a WARN_ON() at runtime.
Fixes: 1b30dbde8596 ("drm: rcar-du: Add support for external pixel clock")
Reported-by: Kuninori Morimoto
Signed-off-by: Laurent Pinchart
---
drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index d18a342626b5..79021d7aa3ce 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -226,9 +226,6 @@ static void rcar_du_crtc_set_display_timing(struct
rcar_du_crtc *rcrtc)
* system clock, and have no internal clock divider.
*/
- if (WARN_ON(!rcrtc->extclock))
- return;
-
/*
* The H3 ES1.x exhibits dot clock duty cycle stability issues.
* We can work around them by configuring the DPLL to twice the
@@ -1113,9 +1110,16 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp,
unsigned int swindex,
clk = devm_clk_get(rcdu->dev, clk_name);
if (!IS_ERR(clk)) {
rcrtc->extclock = clk;
- } else if (PTR_ERR(rcrtc->clock) == -EPROBE_DEFER) {
- dev_info(rcdu->dev, "can't get external clock %u\n", hwindex);
+ } else if (PTR_ERR(clk) == -EPROBE_DEFER) {
return -EPROBE_DEFER;
+ } else if (rcdu->info->dpll_mask & BIT(hwindex)) {
+ /*
+* DU channels that have a display PLL can't use the internal
+* system clock and thus require an external clock.
+*/
+ ret = PTR_ERR(clk);
+ dev_err(rcdu->dev, "can't get dclkin.%u: %d\n", hwindex, ret);
+ return ret;
}
init_waitqueue_head(&rcrtc->flip_wait);
--
Regards,
Laurent Pinchart
Re: HDMI doesn't work on ULCB board
Hi Morimoto-san, On Thursday, 18 October 2018 05:12:14 EET Kuninori Morimoto wrote: > Hi Vladimir, Laurent > > I'm testing ULCB board, and I noticed that HDMI doen't work on it. > > - Sometimes kernel panic happen (I lost log, but it said "get crc" > something error), - Often, kernel doesn't boot with below log if I > connected HDMI cable when booting > - Monitor say "No signal" if I connected cable *after* booting > > I'm using Geert's renesas-drivers-2018-10-09-v4.19-rc7 branch. > It seems salvator HDMI is working correctly on it. > I attached my .config I tried to reproduce the problem, starting with drm-next which seems to work fine, moving to renesas-drivers-2018-10-09-v4.19-rc7 which also didn't exhibit any issue. I then used your kernel configuration, and got a WARN_ON \o/ [1.805110] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). [1.811758] [drm] No driver support for vblank timestamp query. [1.878688] WARNING: CPU: 1 PID: 62 at /home/laurent/src/iob/renesas/linux/ drivers/gpu/drm/rcar-du/rcar_du_crtc.c:229 rcar_du_crtc_get+0x604/0x628 [1.878712] CPU: 1 PID: 62 Comm: kworker/1:1 Not tainted 4.19.0-rc7 #2820 [1.878716] Hardware name: Renesas H3ULCB Kingfisher board based on r8a7795 ES2.0+ (DT) [1.878725] Workqueue: events deferred_probe_work_func [1.878734] pstate: 0005 (nzcv daif -PAN -UAO) [1.878739] pc : rcar_du_crtc_get+0x604/0x628 [1.878743] lr : rcar_du_crtc_get+0xa8/0x628 [1.878747] sp : 0a12b330 [1.878750] x29: 0a12b330 x28: [1.878759] x27: 0004 x26: 8006f90fc000 [1.878768] x25: 8006f9de3ad8 x24: 0001 [1.878776] x23: 0001 x22: 06590fa0 [1.878784] x21: 8006f9026818 x20: 08a6e000 [1.878792] x19: 8006f9610770 x18: 1ef4 [1.878799] x17: x16: 8006faee5900 [1.878807] x15: 1eeb x14: 096a9000 [1.878815] x13: 1ef6 x12: 1eeb [1.878823] x11: 8006faee60e0 x10: 08ab5000 [1.878831] x9 : 098ad000 x8 : 8006faee5900 [1.878839] x7 : 08b1a000 x6 : 08b1a308 [1.878847] x5 : 0002 x4 : 0006 [1.878855] x3 : 087d7670 x2 : 8006f9610018 [1.878862] x1 : x0 : 8006f9611cf0 [1.878871] Call trace: [1.878876] rcar_du_crtc_get+0x604/0x628 [1.878880] rcar_du_crtc_atomic_begin+0x2c/0x60 [1.878890] drm_atomic_helper_commit_planes+0x84/0x218 [1.878895] rcar_du_atomic_commit_tail+0x38/0x70 [1.878900] commit_tail+0x4c/0x80 [1.878904] drm_atomic_helper_commit+0xfc/0x178 [1.878911] drm_atomic_commit+0x54/0x60 [1.878918] restore_fbdev_mode_atomic+0x180/0x1f0 [1.878922] restore_fbdev_mode+0x58/0x198 [1.878927] drm_fb_helper_restore_fbdev_mode_unlocked+0x80/0xd8 [1.878932] drm_fb_helper_set_par+0x34/0x68 [1.878939] fbcon_init+0x504/0x558 [1.878945] visual_init+0xa0/0xf0 [1.878950] do_bind_con_driver+0x160/0x338 [1.878955] do_take_over_console+0x128/0x1a8 [1.878959] do_fbcon_takeover+0x80/0x108 [1.878964] fbcon_event_notify+0x9ac/0x9d8 [1.878971] notifier_call_chain+0x5c/0x98 [1.878975] __blocking_notifier_call_chain+0x58/0xa0 [1.878979] blocking_notifier_call_chain+0x3c/0x50 [1.878983] fb_notifier_call_chain+0x30/0x40 [1.878987] register_framebuffer+0x288/0x330 [1.878992] __drm_fb_helper_initial_config_and_unlock+0x24c/0x410 [1.878996] drm_fb_helper_fbdev_setup+0xf0/0x210 [1.879000] drm_fbdev_cma_init+0xa4/0xf0 [1.879005] rcar_du_modeset_init+0x3b4/0x570 [1.879009] rcar_du_probe+0x134/0x210 [1.879016] platform_drv_probe+0x58/0xb8 [1.879020] really_probe+0x20c/0x2a0 [1.879024] driver_probe_device+0x60/0x108 [1.879028] __device_attach_driver+0x9c/0xe0 [1.879031] bus_for_each_drv+0x80/0xe0 [1.879035] __device_attach+0xf0/0x150 [1.879039] device_initial_probe+0x24/0x30 [1.879042] bus_probe_device+0xa0/0xa8 [1.879046] deferred_probe_work_func+0x78/0xb0 [1.879052] process_one_work+0x2c4/0x6c0 [1.879055] worker_thread+0x244/0x438 [1.879060] kthread+0x100/0x130 [1.879067] ret_from_fork+0x10/0x1c [1.879070] irq event stamp: 42150 [1.879080] hardirqs last enabled at (42149): [] _raw_spin_unlock_irqrestore+0x7c/0x88 [1.879086] hardirqs last disabled at (42150): [] do_debug_exception+0x17c/0x1b8 [1.879090] softirqs last enabled at (41936): [] __do_softirq+0x46c/0x51c [1.879097] softirqs last disabled at (41915): [] irq_exit+0x12c/0x138 [1.879100] ---[ end trace ec07bda5b9bd7ef1 ]--- I'm now investigating it. -- Regards, Laurent Pinchart
Re: [PATCH v4 5/6] media: rcar: rcar-csi2: Update V3M/E3 PHTW tables
Hi Jacopo,
Thank you for the patch.
On Tuesday, 6 November 2018 12:54:26 EET Jacopo Mondi wrote:
> Update PHTW tables for V3M and E3 SoCs to the latest datasheet release
> (R-Car Series, 3rd Generation manual rev1.00 20181017).
>
> Signed-off-by: Jacopo Mondi
>
> ---
> v1 -> v2:
> - Limit the PHTW table to 1125 MBps, according to Laurent's comment
Based on the assumption that the values haven't changed between v1 and v2,
Reviewed-by: Laurent Pinchart
> ---
> drivers/media/platform/rcar-vin/rcar-csi2.c | 62 --
> 1 file changed, 31 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> b/drivers/media/platform/rcar-vin/rcar-csi2.c index 695686b..99f5b76 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -152,37 +152,37 @@ static const struct rcsi2_mbps_reg
> phtw_mbps_h3_v3h_m3n[] = { };
>
> static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
> - { .mbps = 80, .reg = 0x00 },
> - { .mbps = 90, .reg = 0x20 },
> - { .mbps = 100, .reg = 0x40 },
> - { .mbps = 110, .reg = 0x02 },
> - { .mbps = 130, .reg = 0x22 },
> - { .mbps = 140, .reg = 0x42 },
> - { .mbps = 150, .reg = 0x04 },
> - { .mbps = 170, .reg = 0x24 },
> - { .mbps = 180, .reg = 0x44 },
> - { .mbps = 200, .reg = 0x06 },
> - { .mbps = 220, .reg = 0x26 },
> - { .mbps = 240, .reg = 0x46 },
> - { .mbps = 250, .reg = 0x08 },
> - { .mbps = 270, .reg = 0x28 },
> - { .mbps = 300, .reg = 0x0a },
> - { .mbps = 330, .reg = 0x2a },
> - { .mbps = 360, .reg = 0x4a },
> - { .mbps = 400, .reg = 0x0c },
> - { .mbps = 450, .reg = 0x2c },
> - { .mbps = 500, .reg = 0x0e },
> - { .mbps = 550, .reg = 0x2e },
> - { .mbps = 600, .reg = 0x10 },
> - { .mbps = 650, .reg = 0x30 },
> - { .mbps = 700, .reg = 0x12 },
> - { .mbps = 750, .reg = 0x32 },
> - { .mbps = 800, .reg = 0x52 },
> - { .mbps = 850, .reg = 0x72 },
> - { .mbps = 900, .reg = 0x14 },
> - { .mbps = 950, .reg = 0x34 },
> - { .mbps = 1000, .reg = 0x54 },
> - { .mbps = 1050, .reg = 0x74 },
> + { .mbps = 89, .reg = 0x00 },
> + { .mbps = 99, .reg = 0x20 },
> + { .mbps = 109, .reg = 0x40 },
> + { .mbps = 129, .reg = 0x02 },
> + { .mbps = 139, .reg = 0x22 },
> + { .mbps = 149, .reg = 0x42 },
> + { .mbps = 169, .reg = 0x04 },
> + { .mbps = 179, .reg = 0x24 },
> + { .mbps = 199, .reg = 0x44 },
> + { .mbps = 219, .reg = 0x06 },
> + { .mbps = 239, .reg = 0x26 },
> + { .mbps = 249, .reg = 0x46 },
> + { .mbps = 269, .reg = 0x08 },
> + { .mbps = 299, .reg = 0x28 },
> + { .mbps = 329, .reg = 0x0a },
> + { .mbps = 359, .reg = 0x2a },
> + { .mbps = 399, .reg = 0x4a },
> + { .mbps = 449, .reg = 0x0c },
> + { .mbps = 499, .reg = 0x2c },
> + { .mbps = 549, .reg = 0x0e },
> + { .mbps = 599, .reg = 0x2e },
> + { .mbps = 649, .reg = 0x10 },
> + { .mbps = 699, .reg = 0x30 },
> + { .mbps = 749, .reg = 0x12 },
> + { .mbps = 799, .reg = 0x32 },
> + { .mbps = 849, .reg = 0x52 },
> + { .mbps = 899, .reg = 0x72 },
> + { .mbps = 949, .reg = 0x14 },
> + { .mbps = 999, .reg = 0x34 },
> + { .mbps = 1049, .reg = 0x54 },
> + { .mbps = 1099, .reg = 0x74 },
> { .mbps = 1125, .reg = 0x16 },
> { /* sentinel */ },
> };
--
Regards,
Laurent Pinchart
Re: [PATCH] arm64: dts: renesas: r8a77990: Fix VIN endpoint numbering
Hi Jacopo,
Thank you for the patch.
On Monday, 5 November 2018 13:49:21 EET Jacopo Mondi wrote:
> The VIN driver bindings dictates fixed numbering for VIN endpoints connected
> to CSI-2 endpoints, even when a single endpoint exists.
>
> Without proper endpoint numbering the VIN driver fails to probe.
>
> Fixes: ec70407ae7d7 ("arm64: dts: renesas: r8a77990: Add VIN and CSI-2
> device nodes") Signed-off-by: Koji Matsuoka
> Signed-off-by: Takeshi Kihara
> Signed-off-by: Jacopo Mondi
Reviewed-by: Laurent Pinchart
> ---
> arch/arm64/boot/dts/renesas/r8a77990.dtsi | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a77990.dtsi
> b/arch/arm64/boot/dts/renesas/r8a77990.dtsi index 9509dc0..97f7e0c 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77990.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a77990.dtsi
> @@ -747,7 +747,8 @@
> port@1 {
> reg = <1>;
>
> - vin4csi40: endpoint {
> + vin4csi40: endpoint@2 {
> + reg = <2>;
> remote-endpoint= <&csi40vin4>;
> };
> };
> @@ -771,7 +772,8 @@
> port@1 {
> reg = <1>;
>
> - vin5csi40: endpoint {
> + vin5csi40: endpoint@2 {
> + reg = <2>;
> remote-endpoint= <&csi40vin5>;
> };
> };
> --
> 2.7.4
--
Regards,
Laurent Pinchart
Re: [PATCH 5/6] media: rcar: rcar-csi2: Update V3M/E3 PHTW tables
Hi Jacopo,
Thank you for the patch.
On Monday, 5 November 2018 13:19:10 EET Jacopo Mondi wrote:
> Update PHTW tables for V3M and E3 SoCs to the latest datasheet release.
>
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/platform/rcar-vin/rcar-csi2.c | 71 +++---
> 1 file changed, 39 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> b/drivers/media/platform/rcar-vin/rcar-csi2.c index 695686b..5689a60 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -152,38 +152,45 @@ static const struct rcsi2_mbps_reg
> phtw_mbps_h3_v3h_m3n[] = { };
>
> static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
> - { .mbps = 80, .reg = 0x00 },
> - { .mbps = 90, .reg = 0x20 },
> - { .mbps = 100, .reg = 0x40 },
> - { .mbps = 110, .reg = 0x02 },
> - { .mbps = 130, .reg = 0x22 },
> - { .mbps = 140, .reg = 0x42 },
> - { .mbps = 150, .reg = 0x04 },
> - { .mbps = 170, .reg = 0x24 },
> - { .mbps = 180, .reg = 0x44 },
> - { .mbps = 200, .reg = 0x06 },
> - { .mbps = 220, .reg = 0x26 },
> - { .mbps = 240, .reg = 0x46 },
> - { .mbps = 250, .reg = 0x08 },
> - { .mbps = 270, .reg = 0x28 },
> - { .mbps = 300, .reg = 0x0a },
> - { .mbps = 330, .reg = 0x2a },
> - { .mbps = 360, .reg = 0x4a },
> - { .mbps = 400, .reg = 0x0c },
> - { .mbps = 450, .reg = 0x2c },
> - { .mbps = 500, .reg = 0x0e },
> - { .mbps = 550, .reg = 0x2e },
> - { .mbps = 600, .reg = 0x10 },
> - { .mbps = 650, .reg = 0x30 },
> - { .mbps = 700, .reg = 0x12 },
> - { .mbps = 750, .reg = 0x32 },
> - { .mbps = 800, .reg = 0x52 },
> - { .mbps = 850, .reg = 0x72 },
> - { .mbps = 900, .reg = 0x14 },
> - { .mbps = 950, .reg = 0x34 },
> - { .mbps = 1000, .reg = 0x54 },
> - { .mbps = 1050, .reg = 0x74 },
> - { .mbps = 1125, .reg = 0x16 },
> + { .mbps = 89, .reg = 0x00 },
> + { .mbps = 99, .reg = 0x20 },
> + { .mbps = 109, .reg = 0x40 },
> + { .mbps = 129, .reg = 0x02 },
> + { .mbps = 139, .reg = 0x22 },
> + { .mbps = 149, .reg = 0x42 },
> + { .mbps = 169, .reg = 0x04 },
> + { .mbps = 179, .reg = 0x24 },
> + { .mbps = 199, .reg = 0x44 },
> + { .mbps = 219, .reg = 0x06 },
> + { .mbps = 239, .reg = 0x26 },
> + { .mbps = 249, .reg = 0x46 },
> + { .mbps = 269, .reg = 0x08 },
> + { .mbps = 299, .reg = 0x28 },
> + { .mbps = 329, .reg = 0x0a },
> + { .mbps = 359, .reg = 0x2a },
> + { .mbps = 399, .reg = 0x4a },
> + { .mbps = 449, .reg = 0x0c },
> + { .mbps = 499, .reg = 0x2c },
> + { .mbps = 549, .reg = 0x0e },
> + { .mbps = 599, .reg = 0x2e },
> + { .mbps = 649, .reg = 0x10 },
> + { .mbps = 699, .reg = 0x30 },
> + { .mbps = 749, .reg = 0x12 },
> + { .mbps = 799, .reg = 0x32 },
> + { .mbps = 849, .reg = 0x52 },
> + { .mbps = 899, .reg = 0x72 },
> + { .mbps = 949, .reg = 0x14 },
> + { .mbps = 999, .reg = 0x34 },
> + { .mbps = 1049, .reg = 0x54 },
> + { .mbps = 1099, .reg = 0x74 },
> + { .mbps = 1149, .reg = 0x16 },
> + { .mbps = 1199, .reg = 0x36 },
> + { .mbps = 1249, .reg = 0x56 },
> + { .mbps = 1299, .reg = 0x76 },
> + { .mbps = 1349, .reg = 0x18 },
> + { .mbps = 1399, .reg = 0x38 },
> + { .mbps = 1449, .reg = 0x58 },
> + { .mbps = 1500, .reg = 0x78 },
> { /* sentinel */ },
> };
In the latest datasheet version I can find, the frequencies go up to 1125 MHz
only. I've verified values up to that point, but not beyond it.
--
Regards,
Laurent Pinchart
Re: [PATCH 6/6] media: rcar-csi2: Handle per-SoC number of channels
Hi Jacopo,
Thank you for the patch.
On Monday, 5 November 2018 13:19:11 EET Jacopo Mondi wrote:
> The R-Car CSI-2 interface has a number of selectable 'channels' that
> provides pixel data to the VINs during image acquisition.
>
> Each channel can be used to match a CSI-2 data type and a CSI-2 virtual
> channel to be routed to output path.
>
> Different SoCs have different number of channels, with R-Car E3 being the
> notable exception supporting only 2 of them.
>
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/platform/rcar-vin/rcar-csi2.c | 15 ---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> b/drivers/media/platform/rcar-vin/rcar-csi2.c index 5689a60..95a3dd4 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -349,6 +349,7 @@ struct rcar_csi2_info {
> int (*confirm_start)(struct rcar_csi2 *priv);
> const struct rcsi2_mbps_reg *hsfreqrange;
> unsigned int csi0clkfreqrange;
> + unsigned int num_channels;
> bool clear_ulps;
> };
>
> @@ -483,13 +484,14 @@ static int rcsi2_start(struct rcar_csi2 *priv)
> format = rcsi2_code_to_fmt(priv->mf.code);
>
> /*
> - * Enable all Virtual Channels.
> + * Enable all supported CSI-2 channels with virtual channel and
> + * data type matching.
>*
>* NOTE: It's not possible to get individual datatype for each
>* source virtual channel. Once this is possible in V4L2
>* it should be used here.
>*/
> - for (i = 0; i < 4; i++) {
> + for (i = 0; i < priv->info->num_channels; i++) {
> u32 vcdt_part;
>
> vcdt_part = VCDT_SEL_VC(i) | VCDT_VCDTN_EN | VCDT_SEL_DTN_ON |
> @@ -518,7 +520,8 @@ static int rcsi2_start(struct rcar_csi2 *priv)
> rcsi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 |
> FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN);
> rcsi2_write(priv, VCDT_REG, vcdt);
> - rcsi2_write(priv, VCDT2_REG, vcdt2);
> + if (vcdt2)
I would write this as
if (priv->info->num_channels > 2)
in order to later support configuration of virtual channels and data types
that could result in the output channels 2 and 3 being disabled.
Apart from this,
Reviewed-by: Laurent Pinchart
> + rcsi2_write(priv, VCDT2_REG, vcdt2);
> /* Lanes are zero indexed. */
> rcsi2_write(priv, LSWAP_REG,
> LSWAP_L0SEL(priv->lane_swap[0] - 1) |
> @@ -947,32 +950,38 @@ static const struct rcar_csi2_info
> rcar_csi2_info_r8a7795 = { .init_phtw = rcsi2_init_phtw_h3_v3h_m3n,
> .hsfreqrange = hsfreqrange_h3_v3h_m3n,
> .csi0clkfreqrange = 0x20,
> + .num_channels = 4,
> .clear_ulps = true,
> };
>
> static const struct rcar_csi2_info rcar_csi2_info_r8a7795es1 = {
> .hsfreqrange = hsfreqrange_m3w_h3es1,
> + .num_channels = 4,
> };
>
> static const struct rcar_csi2_info rcar_csi2_info_r8a7796 = {
> .hsfreqrange = hsfreqrange_m3w_h3es1,
> + .num_channels = 4,
> };
>
> static const struct rcar_csi2_info rcar_csi2_info_r8a77965 = {
> .init_phtw = rcsi2_init_phtw_h3_v3h_m3n,
> .hsfreqrange = hsfreqrange_h3_v3h_m3n,
> .csi0clkfreqrange = 0x20,
> + .num_channels = 4,
> .clear_ulps = true,
> };
>
> static const struct rcar_csi2_info rcar_csi2_info_r8a77970 = {
> .init_phtw = rcsi2_init_phtw_v3m_e3,
> .confirm_start = rcsi2_confirm_start_v3m_e3,
> + .num_channels = 4,
> };
>
> static const struct rcar_csi2_info rcar_csi2_info_r8a77990 = {
> .init_phtw = rcsi2_init_phtw_v3m_e3,
> .confirm_start = rcsi2_confirm_start_v3m_e3,
> + .num_channels = 2,
> };
>
> static const struct of_device_id rcar_csi2_of_table[] = {
--
Regards,
Laurent Pinchart
Re: [PATCH v3 4/4] i2c: adv748x: configure number of lanes used for TXA CSI-2 transmitter
Hi Niklas,
Thank you for the patch.
On Friday, 2 November 2018 18:00:09 EET Niklas Söderlund wrote:
> The driver fixed the TXA CSI-2 transmitter in 4-lane mode while it could
> operate using 1-, 2- and 4-lanes. Update the driver to support all modes
> the hardware does.
"all modes the hardware does" sounds weird. How about simple "all available
modes" ?
> The driver make use of large tables of static register/value writes when
s/make/makes/
> powering up/down the TXA and TXB transmitters which include the write to
> the NUM_LANES register. By converting the tables into functions and
> using parameters the power up/down functions for TXA and TXB power
> up/down can be merged and used for both transmitters.
>
> Signed-off-by: Niklas Söderlund
Reviewed-by: Laurent Pinchart
The code looks so much better now :-)
> ---
> * Changes since v2
> - Fix typos in comments.
> - Remove unneeded boiler plait code in adv748x_tx_power() as suggested
> by Jacopo and Laurent.
> - Take into account the two different register used when powering up TXA
> and TXB due to an earlier patch in this series aligns the power
> sequence with the manual.
>
> * Changes since v1
> - Convert tables of register/value writes into functions instead of
> intercepting and modifying the writes to the NUM_LANES register.
> ---
> drivers/media/i2c/adv748x/adv748x-core.c | 157 ---
> 1 file changed, 79 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> b/drivers/media/i2c/adv748x/adv748x-core.c index
> 9d80d7f3062b16bc..d94c63cb6a2efdba 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -125,6 +125,16 @@ int adv748x_write(struct adv748x_state *state, u8 page,
> u8 reg, u8 value) return regmap_write(state->regmap[page], reg, value);
> }
>
> +static int adv748x_write_check(struct adv748x_state *state, u8 page, u8
> reg,
> +u8 value, int *error)
> +{
> + if (*error)
> + return *error;
> +
> + *error = adv748x_write(state, page, reg, value);
> + return *error;
> +}
> +
> /* adv748x_write_block(): Write raw data with a maximum of
> I2C_SMBUS_BLOCK_MAX * size to one or more registers.
> *
> @@ -231,79 +241,77 @@ static int adv748x_write_regs(struct adv748x_state
> *state, * TXA and TXB
> */
>
> -static const struct adv748x_reg_value adv748x_power_up_txa_4lane[] = {
> -
> - {ADV748X_PAGE_TXA, 0x00, 0x84}, /* Enable 4-lane MIPI */
> - {ADV748X_PAGE_TXA, 0x00, 0xa4}, /* Set Auto DPHY Timing */
> - {ADV748X_PAGE_TXA, 0xdb, 0x10}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0xd6, 0x07}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0xc4, 0x0a}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0x71, 0x33}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0x72, 0x11}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0xf0, 0x00}, /* i2c_dphy_pwdn - 1'b0 */
> -
> - {ADV748X_PAGE_TXA, 0x31, 0x82}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0x1e, 0x40}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0xda, 0x01}, /* i2c_mipi_pll_en - 1'b1 */
> - {ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> - {ADV748X_PAGE_TXA, 0x00, 0x24 },/* Power-up CSI-TX */
> - {ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> - {ADV748X_PAGE_TXA, 0xc1, 0x2b}, /* ADI Required Write */
> - {ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> - {ADV748X_PAGE_TXA, 0x31, 0x80}, /* ADI Required Write */
> -
> - {ADV748X_PAGE_EOR, 0xff, 0xff} /* End of register table */
> -};
> -
> -static const struct adv748x_reg_value adv748x_power_down_txa_4lane[] = {
> -
> - {ADV748X_PAGE_TXA, 0x31, 0x82}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0x1e, 0x00}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0x00, 0x84}, /* Enable 4-lane MIPI */
> - {ADV748X_PAGE_TXA, 0xda, 0x01}, /* i2c_mipi_pll_en - 1'b1 */
> - {ADV748X_PAGE_TXA, 0xc1, 0x3b}, /* ADI Required Write */
> -
> - {ADV748X_PAGE_EOR, 0xff, 0xff} /* End of register table */
> -};
> -
> -static const struct adv748x_reg_value adv748x_power_up_txb_1lane[] = {
> -
> - {ADV748X_PAGE_TXB, 0x00, 0x81}, /* Enable 1-lane MIPI */
> - {ADV748X_PAGE_TXB, 0x00, 0xa1}, /* Set Auto DPHY Timing */
> - {ADV748X_PAGE_TXB, 0xd2, 0x40}, /* ADI Required Write */
> - {ADV748X_PAGE_TXB, 0xc4, 0x0a}, /* ADI Required Write */
> - {ADV748X_PAGE_TXB, 0x71, 0x33}, /* ADI Required Write */
> - {ADV748X_PAGE_TXB, 0x72, 0x11}, /* ADI Required Write */
> - {ADV748X_PAGE_TXB, 0xf0, 0x00}, /* i2c_dphy_pwdn - 1'b0 */
> -
>
Re: [PATCH v3 3/4] i2c: adv748x: store number of CSI-2 lanes described in device tree
Hi Niklas,
Thank you for the patch.
On Friday, 2 November 2018 18:00:08 EET Niklas Söderlund wrote:
> The adv748x CSI-2 transmitters TXA and TXB can use different number of
> lanes to transmit data. In order to be able to configure the device
> correctly this information need to be parsed from device tree and stored
> in each TX private data structure.
>
> TXA supports 1, 2 and 4 lanes while TXB supports 1 lane.
>
> Signed-off-by: Niklas Söderlund
>
> ---
> * Changes since v2
> - Rebase to latest media-tree requires the bus_type filed in struct
> v4l2_fwnode_endpoint to be initialized, set it to V4L2_MBUS_CSI2_DPHY.
>
> * Changes since v1
> - Use %u instead of %d to print unsigned int.
> - Fix spelling in commit message, thanks Laurent.
> ---
> drivers/media/i2c/adv748x/adv748x-core.c | 50
> drivers/media/i2c/adv748x/adv748x.h | 1 +
> 2 files changed, 51 insertions(+)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> b/drivers/media/i2c/adv748x/adv748x-core.c index
> 2384f50dacb0ccff..9d80d7f3062b16bc 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -23,6 +23,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include "adv748x.h"
> @@ -521,12 +522,56 @@ void adv748x_subdev_init(struct v4l2_subdev *sd,
> struct adv748x_state *state, sd->entity.ops = &adv748x_media_ops;
> }
>
> +static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
> + unsigned int port,
> + struct device_node *ep)
> +{
> + struct v4l2_fwnode_endpoint vep;
> + unsigned int num_lanes;
> + int ret;
> +
> + if (port != ADV748X_PORT_TXA && port != ADV748X_PORT_TXB)
> + return 0;
> +
> + vep.bus_type = V4L2_MBUS_CSI2_DPHY;
> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &vep);
> + if (ret)
> + return ret;
> +
> + num_lanes = vep.bus.mipi_csi2.num_data_lanes;
> +
> + if (vep.base.port == ADV748X_PORT_TXA) {
> + if (num_lanes != 1 && num_lanes != 2 && num_lanes != 4) {
> + adv_err(state, "TXA: Invalid number (%u) of lanes\n",
> + num_lanes);
> + return -EINVAL;
> + }
> +
> + state->txa.num_lanes = num_lanes;
> + adv_dbg(state, "TXA: using %u lanes\n", state->txa.num_lanes);
> + }
> +
> + if (vep.base.port == ADV748X_PORT_TXB) {
> + if (num_lanes != 1) {
> + adv_err(state, "TXB: Invalid number (%u) of lanes\n",
> + num_lanes);
> + return -EINVAL;
> + }
> +
> + state->txb.num_lanes = num_lanes;
> + adv_dbg(state, "TXB: using %u lanes\n", state->txb.num_lanes);
> + }
Should we set the number of lanes to 4 and 1 respectively by default to
support old DTs ? Apart from that,
Reviewed-by: Laurent Pinchart
> + return 0;
> +}
> +
> static int adv748x_parse_dt(struct adv748x_state *state)
> {
> struct device_node *ep_np = NULL;
> struct of_endpoint ep;
> bool out_found = false;
> bool in_found = false;
> + int ret;
>
> for_each_endpoint_of_node(state->dev->of_node, ep_np) {
> of_graph_parse_endpoint(ep_np, &ep);
> @@ -557,6 +602,11 @@ static int adv748x_parse_dt(struct adv748x_state
> *state) in_found = true;
> else
> out_found = true;
> +
> + /* Store number of CSI-2 lanes used for TXA and TXB. */
> + ret = adv748x_parse_csi2_lanes(state, ep.port, ep_np);
> + if (ret)
> + return ret;
> }
>
> return in_found && out_found ? 0 : -ENODEV;
> diff --git a/drivers/media/i2c/adv748x/adv748x.h
> b/drivers/media/i2c/adv748x/adv748x.h index
> 39c2fdc3b41667d8..b482c7fe6957eb85 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -79,6 +79,7 @@ struct adv748x_csi2 {
> struct v4l2_mbus_framefmt format;
> unsigned int page;
> unsigned int port;
> + unsigned int num_lanes;
>
> struct media_pad pads[ADV748X_CSI2_NR_PADS];
> struct v4l2_ctrl_handler ctrl_hdl;
--
Regards,
Laurent Pinchart
Re: [PATCH v3 2/4] i2c: adv748x: reuse power up sequence when initializing CSI-2
Hi Niklas,
Thank you for the patch.
On Friday, 2 November 2018 18:00:07 EET Niklas Söderlund wrote:
> Extend the MIPI CSI-2 power up sequence to match the power up sequence
> in the hardware manual chapter "9.5.1 Power Up Sequence". This change
> allows the power up functions to be reused when initializing the
> hardware reducing code duplicating as well aligning with the
> documentation.
>
> Signed-off-by: Niklas Söderlund
Reviewed-by: Laurent Pinchart
>
> ---
> * Changes since v2
> - Bring in the undocumented registers in the power on sequence from the
> initialization sequence after confirming in the hardware manual that
> this is the correct behavior.
> ---
> drivers/media/i2c/adv748x/adv748x-core.c | 50 ++--
> 1 file changed, 13 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> b/drivers/media/i2c/adv748x/adv748x-core.c index
> 6854d898fdd1f192..2384f50dacb0ccff 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -234,6 +234,12 @@ static const struct adv748x_reg_value
> adv748x_power_up_txa_4lane[] = {
>
> {ADV748X_PAGE_TXA, 0x00, 0x84}, /* Enable 4-lane MIPI */
> {ADV748X_PAGE_TXA, 0x00, 0xa4}, /* Set Auto DPHY Timing */
> + {ADV748X_PAGE_TXA, 0xdb, 0x10}, /* ADI Required Write */
> + {ADV748X_PAGE_TXA, 0xd6, 0x07}, /* ADI Required Write */
> + {ADV748X_PAGE_TXA, 0xc4, 0x0a}, /* ADI Required Write */
> + {ADV748X_PAGE_TXA, 0x71, 0x33}, /* ADI Required Write */
> + {ADV748X_PAGE_TXA, 0x72, 0x11}, /* ADI Required Write */
> + {ADV748X_PAGE_TXA, 0xf0, 0x00}, /* i2c_dphy_pwdn - 1'b0 */
>
> {ADV748X_PAGE_TXA, 0x31, 0x82}, /* ADI Required Write */
> {ADV748X_PAGE_TXA, 0x1e, 0x40}, /* ADI Required Write */
> @@ -263,6 +269,11 @@ static const struct adv748x_reg_value
> adv748x_power_up_txb_1lane[] = {
>
> {ADV748X_PAGE_TXB, 0x00, 0x81}, /* Enable 1-lane MIPI */
> {ADV748X_PAGE_TXB, 0x00, 0xa1}, /* Set Auto DPHY Timing */
> + {ADV748X_PAGE_TXB, 0xd2, 0x40}, /* ADI Required Write */
> + {ADV748X_PAGE_TXB, 0xc4, 0x0a}, /* ADI Required Write */
> + {ADV748X_PAGE_TXB, 0x71, 0x33}, /* ADI Required Write */
> + {ADV748X_PAGE_TXB, 0x72, 0x11}, /* ADI Required Write */
> + {ADV748X_PAGE_TXB, 0xf0, 0x00}, /* i2c_dphy_pwdn - 1'b0 */
>
> {ADV748X_PAGE_TXB, 0x31, 0x82}, /* ADI Required Write */
> {ADV748X_PAGE_TXB, 0x1e, 0x40}, /* ADI Required Write */
> @@ -383,25 +394,6 @@ static const struct adv748x_reg_value
> adv748x_init_txa_4lane[] = { {ADV748X_PAGE_IO, 0x0c, 0xe0}, /* Enable
> LLC_DLL & Double LLC Timing */ {ADV748X_PAGE_IO, 0x0e, 0xdd}, /*
> LLC/PIX/SPI PINS TRISTATED AUD */
>
> - {ADV748X_PAGE_TXA, 0x00, 0x84}, /* Enable 4-lane MIPI */
> - {ADV748X_PAGE_TXA, 0x00, 0xa4}, /* Set Auto DPHY Timing */
> - {ADV748X_PAGE_TXA, 0xdb, 0x10}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0xd6, 0x07}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0xc4, 0x0a}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0x71, 0x33}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0x72, 0x11}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0xf0, 0x00}, /* i2c_dphy_pwdn - 1'b0 */
> -
> - {ADV748X_PAGE_TXA, 0x31, 0x82}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0x1e, 0x40}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0xda, 0x01}, /* i2c_mipi_pll_en - 1'b1 */
> - {ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> - {ADV748X_PAGE_TXA, 0x00, 0x24 },/* Power-up CSI-TX */
> - {ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> - {ADV748X_PAGE_TXA, 0xc1, 0x2b}, /* ADI Required Write */
> - {ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> - {ADV748X_PAGE_TXA, 0x31, 0x80}, /* ADI Required Write */
> -
> {ADV748X_PAGE_EOR, 0xff, 0xff} /* End of register table */
> };
>
> @@ -435,24 +427,6 @@ static const struct adv748x_reg_value
> adv748x_init_txb_1lane[] = { {ADV748X_PAGE_SDP, 0x31, 0x12}, /* ADI
> Required Write */
> {ADV748X_PAGE_SDP, 0xe6, 0x4f}, /* V bit end pos manually in NTSC */
>
> - {ADV748X_PAGE_TXB, 0x00, 0x81}, /* Enable 1-lane MIPI */
> - {ADV748X_PAGE_TXB, 0x00, 0xa1}, /* Set Auto DPHY Timing */
> - {ADV748X_PAGE_TXB, 0xd2, 0x40}, /* ADI Required Write */
> - {ADV748X_PAGE_TXB, 0xc4, 0x0a}, /* ADI Required Write */
> - {ADV748X_PAGE_TXB, 0x71, 0x33}, /* ADI Required Write */
> - {ADV748X_PAGE_TXB, 0x72, 0x11}, /* ADI Required Write */
> - {ADV748X_PAGE_TXB, 0xf0, 0x00}, /* i2c_dphy_pwdn - 1'b0 */
> - {ADV748X_PAGE_TXB, 0x31, 0x82}, /* ADI Required Write *
Re: [PATCH v3 1/4] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints
Hi Niklas, Thank you for the patch. On Friday, 2 November 2018 18:00:06 EET Niklas Söderlund wrote: > The CSI-2 transmitters can use a different number of lanes to transmit > data. Make the data-lanes mandatory for the endpoints that describe the > transmitters as no good default can be set to fallback on. > > Signed-off-by: Niklas Söderlund Reviewed-by: Laurent Pinchart > > --- > * Changes since v2 > - Update paragraph according to Laurents comments. > --- > Documentation/devicetree/bindings/media/i2c/adv748x.txt | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt > b/Documentation/devicetree/bindings/media/i2c/adv748x.txt index > 5dddc95f9cc46084..bffbabc879efd86c 100644 > --- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt > +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt > @@ -48,7 +48,9 @@ are numbered as follows. > TXA source 10 > TXB source 11 > > -The digital output port nodes must contain at least one endpoint. > +The digital output port nodes, when present, shall contain at least one > +endpoint. Each of those endpoints shall contain the data-lanes property as > +described in video-interfaces.txt. > > Ports are optional if they are not connected to anything at the hardware > level. -- Regards, Laurent Pinchart
Re: [PATCH v2 2/5] i2c: adv748x: reorder register writes for CSI-2 transmitters initialization
Hi Niklas,
On Friday, 2 November 2018 12:38:34 EET Niklas Söderlund wrote:
> On 2018-10-05 01:36:11 +0300, Laurent Pinchart wrote:
> > On Thursday, 4 October 2018 23:41:35 EEST Niklas Söderlund wrote:
> > > From: Niklas Söderlund
> > >
> > > Reorder the initialization order of registers to allow for refactoring.
> > > The move could have been done at the same time as the refactoring but
> > > since the documentation about some registers involved are missing do it
> > > separately.
> > >
> > > Signed-off-by: Niklas Söderlund
> > > ---
> > >
> > > drivers/media/i2c/adv748x/adv748x-core.c | 12 +++-
> > > 1 file changed, 7 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> > > b/drivers/media/i2c/adv748x/adv748x-core.c index
> > > 6854d898fdd1f192..721ed6552bc1cde6 100644
> > > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > > @@ -383,8 +383,6 @@ static const struct adv748x_reg_value
> > > adv748x_init_txa_4lane[] = { {ADV748X_PAGE_IO, 0x0c, 0xe0}, /*
> > > Enable
> > > LLC_DLL & Double LLC Timing */ {ADV748X_PAGE_IO, 0x0e, 0xdd}, /*
> > > LLC/PIX/SPI PINS TRISTATED AUD */
> > >
> > > - {ADV748X_PAGE_TXA, 0x00, 0x84}, /* Enable 4-lane MIPI */
> > > - {ADV748X_PAGE_TXA, 0x00, 0xa4}, /* Set Auto DPHY Timing */
> > >
> > > {ADV748X_PAGE_TXA, 0xdb, 0x10}, /* ADI Required Write */
> > > {ADV748X_PAGE_TXA, 0xd6, 0x07}, /* ADI Required Write */
> > > {ADV748X_PAGE_TXA, 0xc4, 0x0a}, /* ADI Required Write */
> > >
> > > @@ -392,6 +390,9 @@ static const struct adv748x_reg_value
> > > adv748x_init_txa_4lane[] = { {ADV748X_PAGE_TXA, 0x72, 0x11}, /* ADI
> > > Required Write */
> > >
> > > {ADV748X_PAGE_TXA, 0xf0, 0x00}, /* i2c_dphy_pwdn - 1'b0 */
> > >
> > > + {ADV748X_PAGE_TXA, 0x00, 0x84}, /* Enable 4-lane MIPI */
> > > + {ADV748X_PAGE_TXA, 0x00, 0xa4}, /* Set Auto DPHY Timing */
> > > +
> > >
> > > {ADV748X_PAGE_TXA, 0x31, 0x82}, /* ADI Required Write */
> > > {ADV748X_PAGE_TXA, 0x1e, 0x40}, /* ADI Required Write */
> > > {ADV748X_PAGE_TXA, 0xda, 0x01}, /* i2c_mipi_pll_en - 1'b1 */
> > >
> > > @@ -435,17 +436,18 @@ static const struct adv748x_reg_value
> > > adv748x_init_txb_1lane[] = { {ADV748X_PAGE_SDP, 0x31, 0x12}, /* ADI
> > > Required Write */
> > >
> > > {ADV748X_PAGE_SDP, 0xe6, 0x4f}, /* V bit end pos manually in NTSC
*/
> > >
> > > - {ADV748X_PAGE_TXB, 0x00, 0x81}, /* Enable 1-lane MIPI */
> > > - {ADV748X_PAGE_TXB, 0x00, 0xa1}, /* Set Auto DPHY Timing */
> > >
> > > {ADV748X_PAGE_TXB, 0xd2, 0x40}, /* ADI Required Write */
> > > {ADV748X_PAGE_TXB, 0xc4, 0x0a}, /* ADI Required Write */
> > > {ADV748X_PAGE_TXB, 0x71, 0x33}, /* ADI Required Write */
> > > {ADV748X_PAGE_TXB, 0x72, 0x11}, /* ADI Required Write */
> > > {ADV748X_PAGE_TXB, 0xf0, 0x00}, /* i2c_dphy_pwdn - 1'b0 */
> > >
> > > +
> > > + {ADV748X_PAGE_TXB, 0x00, 0x81}, /* Enable 1-lane MIPI */
> > > + {ADV748X_PAGE_TXB, 0x00, 0xa1}, /* Set Auto DPHY Timing */
> > > +
> >
> > This is pretty hard to review, as there's a bunch of undocumented register
> > writes. I think the first write is safe, as the tables are written
> > immediately following a software reset, and the default value of the
> > register is 0x81 (CSI-TX disabled, 1 lane). The second write, however,
> > enables usage of the computed DPHY parameters, and I don't know whether
> > the undocumented register writes in-between may interact with that.
>
> I agree it's hard to grasp all implications with undocumented registers
> involved. That is why I choose to do it in a separate commit so if
> regressions are found it could be bisectable to this change.
>
> > That being said, this change enables further important refactoring, so I'm
> > tempted to accept it. I assume you've tested it and haven't noticed a
> > regression. The part that still bothers me in particular is that the write
> > to register 0xf0 just above this takes the DPHY out of power down
> > according to the datasheet, and I wonder whether at that point the DPHY
> > might not react to that information. Have you analyzed the power-up
> > sequence in section 9.5.1 of the hardware manual ? I wonder whether the
> > dphy_pwdn s
Re: [PATCH v2 6/8] arm64: dts: r8a77990: Add VIN and CSI-2 device nodes
Hi Jacopo,
On Tuesday, 30 October 2018 12:14:31 EET jacopo mondi wrote:
> On Mon, Sep 10, 2018 at 05:12:30PM +0300, Laurent Pinchart wrote:
> > On Wednesday, 5 September 2018 18:29:43 EEST Jacopo Mondi wrote:
> >> From: Koji Matsuoka
> >>
> >> Add device nodes for VIN4, VIN5 and CSI40 to R-Car E3 R8A77990 device
> >> tree.
> >>
> >> Signed-off-by: Koji Matsuoka
> >> Signed-off-by: Takeshi Kihara
> >> Signed-off-by: Jacopo Mondi
> >> ---
> >>
> >> arch/arm64/boot/dts/renesas/r8a77990.dtsi | 79
> >> 1 file changed, 79 insertions(+)
> >>
> >> diff --git a/arch/arm64/boot/dts/renesas/r8a77990.dtsi
> >> b/arch/arm64/boot/dts/renesas/r8a77990.dtsi index ae89260..0ae7bbe
> >> 100644
> >> --- a/arch/arm64/boot/dts/renesas/r8a77990.dtsi
> >> +++ b/arch/arm64/boot/dts/renesas/r8a77990.dtsi
> >> @@ -337,6 +337,85 @@
> >>
> >>status = "disabled";
> >>
> >>};
> >>
> >> + csi40: csi2@feaa {
> >
> > I believe Simon would like to keep the nodes sorted by address
> >
> >> + compatible = "renesas,r8a77990-csi2",
> >> "renesas,rcar-gen3-csi2";
> >> + reg = <0 0xfeaa 0 0x1>;
> >
> > 0x1 seems pretty large to me.
>
> It seems to me that all Gen3 SoC have this lenght specified
>
> $git grep -A 10 'csi[2|4][0|1]: csi' arch/arm64/boot/dts/renesas/ | grep reg
> arch/arm64/boot/dts/renesas/r8a7795.dtsi-reg = <0 0xfea8 0 0x1>;
> arch/arm64/boot/dts/renesas/r8a7795.dtsi-reg = <0 0xfeaa 0 0x1>;
> arch/arm64/boot/dts/renesas/r8a7795.dtsi-reg = <0 0xfeab 0 0x1>;
> arch/arm64/boot/dts/renesas/r8a7796.dtsi-reg = <0 0xfea8 0 0x1>;
> arch/arm64/boot/dts/renesas/r8a7796.dtsi-reg = <0 0xfeaa 0 0x1>;
> arch/arm64/boot/dts/renesas/r8a77965.dtsi- reg = <0 0xfea8 0 0x1>;
> arch/arm64/boot/dts/renesas/r8a77965.dtsi- reg = <0 0xfeaa 0 0x1>;
> arch/arm64/boot/dts/renesas/r8a77970.dtsi- reg = <0 0xfeaa 0 0x1>;
> arch/arm64/boot/dts/renesas/r8a77980.dtsi- reg = <0 0xfeaa 0 0x1>;
> arch/arm64/boot/dts/renesas/r8a77980.dtsi- reg = <0 0xfeab 0 0x1>;
> arch/arm64/boot/dts/renesas/r8a77990.dtsi- reg = <0 0xfeaa 0 0x1>;
>
> Am I missing something?
Cargo-cult programming ? :-) This will likely not hurt, but such a large
memory area is not required, and we'll save a bit of memory if we reduce the
mapping from 64kB to 4kB (or less)
> > Apart from that,
>
> I will include the upporting of the following patch to fix the VIN
> endpoint numbering in forthcoming v3:
> https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit
> /?id=1b1b73f7558d867d72e198901b84bec1e6ef1405
>
> As this has already been applied to simon's tree.
>
> > Reviewed-by: Laurent Pinchart
> >
> >> + interrupts = ;
> >> + clocks = <&cpg CPG_MOD 716>;
> >> + power-domains = <&sysc R8A77990_PD_ALWAYS_ON>;
> >> + resets = <&cpg 716>;
> >> + status = "disabled";
> >> +
> >> + ports {
> >> + #address-cells = <1>;
> >> + #size-cells = <0>;
> >> +
> >> + port@1 {
> >> + #address-cells = <1>;
> >> + #size-cells = <0>;
> >> +
> >> + reg = <1>;
> >> +
> >> + csi40vin4: endpoint@0 {
> >> + reg = <0>;
> >> + remote-endpoint = <&vin4csi40>;
> >> + };
> >> + csi40vin5: endpoint@1 {
> >> + reg = <1>;
> >> + remote-endpoint = <&vin5csi40>;
> >> + };
> >> + };
> >> + };
> >> +
Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume()
Hi Daniel, On Tuesday, 23 October 2018 16:40:41 EEST Daniel Vetter wrote: > On Mon, Oct 22, 2018 at 01:51:35PM +0530, Souptick Joarder wrote: > > On Thu, Oct 11, 2018 at 1:48 PM Daniel Vetter wrote: > >> On Mon, Oct 08, 2018 at 09:57:52PM +0530, Souptick Joarder wrote: > >>> On Mon, Oct 1, 2018 at 6:12 PM Noralf Trønnes wrote: > >>>> Den 01.10.2018 13.56, skrev Laurent Pinchart: > >>>>> On Monday, 1 October 2018 09:52:20 EEST Daniel Vetter wrote: > >>>>>> On Fri, Sep 28, 2018 at 06:30:35PM +0300, Laurent Pinchart wrote: > >>>>>>> On Friday, 28 September 2018 18:05:18 EEST Laurent Pinchart wrote: > >>>>>>>> On Thursday, 27 September 2018 09:34:18 EEST Souptick Joarder wrote: > >>>>>>>>> On Tue, Sep 18, 2018 at 10:05 PM Souptick Joarder wrote: > >>>>>>>>>> convert drm_atomic_helper_suspend/resume() to use > >>>>>>>>>> drm_mode_config_helper_suspend/resume(). > >>>>>>>>>> > >>>>>>>>>> remove suspend_state field from the rcar_du_device > >>>>>>>>>> structure as it is no more required. > >>>>>>>>>> > >>>>>>>>>> With this conversion, also > >>>>>>>>>> drm_fbdev_cma_set_suspend_unlocked() > >>>>>>>>>> will left with no consumer. So this function can be removed. > >>>>>>>>>> > >>>>>>>>>> Signed-off-by: Souptick Joarder > >>>>>>>>> > >>>>>>>>> Laurent, any comment on this patch ?? > >>>>>>>> > >>>>>>>> Sorry for the delay, and thanks for pinging me. > >>>>>>>> > >>>>>>>> Reviewed-by: Laurent Pinchart > >>>>>>>> > >>>>>>>> > >>>>>>>> Should I apply this to my tree or do you plan to merge it > >>>>>>>> through drm-misc as it touches drm_fb_cma_helper.c ? > >>>>>>> > >>>>>>> I just realized that the same patch got supplied by Noralf > >>>>>>> Trønnes nearly a year ago, and was later superseded by > >>>>>>> https://patchwork.freedesktop.org/patch/247861/. I think we > >>>>>>> should thus apply Noralf's patches instead (once he sends v4 > >>>>>>> out). > >>>>>> > >>>>>> I don't think Noralf is still actively working on this, he's busy > >>>>>> with the fbdev emulation stuff. Probably better if you pick up > >>>>>> one of the existing ones, than waiting another year or so :-) > >>>>> > >>>>> Isn't https://patchwork.freedesktop.org/patch/247861/ part of the > >>>>> fbdev emulation stuff ? That patch series is from less than a > >>>>> month ago, so I thought it was actively developed. > >>>>> > >>>>> Let's ask him. Noralf, any comment ? :-) > >>>> > >>>> I see now that I've caused some confusion here. > >>>> > >>>> I did some refactoring of the fbdev stuff in the CMA helper last > >>>> year. In the last series of that work there is this patch: > >>>> > >>>> [v3,07/11] drm/rcar-du: Use drm_mode_config_helper_suspend/resume() > >>>> https://patchwork.freedesktop.org/patch/192414/ > >>>> > >>>> During Christmas I got the idea that maybe it was possible to use a > >>>> dumb buffer to do generic fbdev emulation. That work took me 6 > >>>> months before a solution using a general internal DRM client was > >>>> hammered out. By the time I started to convert drivers I had > >>>> forgotten about that patch. > >>>> > >>>> When I wrote the patch (#247861) to convert rcar to the generic > >>>> fbdev, I just bundled up the suspend/resume change with the fbdev > >>>> conversion. Laurent and Sam commented on that bundling, so I said > >>>> that I would split it up in the next version. > >>>> > >>>> All that being said, I think that this patch should be applied and > >>>> I'll just rebase the rcar fbdev conversion patch on top of that. > >>>> It's the easiest for me at least :-) > >>> > >>> Any conclusion on this patch ? :-) > >> > >> Laurent should pick it up and apply to his rcar-du tree I think. We can > >> also stuff it into drm-misc as a fallback. > > > > Can we get this patch in queue for 4.20 ? > > 5.1, 4.20 is already done. > > But yes, I pulled this into drm-misc-next. Thanks for your patch. I would have preferred applying this through my tree. I'm currently at ELCE and had to prepare for the conference, hence the delay. > >>>>>>>>>> drivers/gpu/drm/drm_fb_cma_helper.c | 18 -- > >>>>>>>>>> drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 ++- > >>>>>>>>>> drivers/gpu/drm/rcar-du/rcar_du_drv.h | 1 - > >>>>>>>>>> include/drm/drm_fb_cma_helper.h | 2 -- > >>>>>>>>>> 4 files changed, 2 insertions(+), 40 deletions(-) -- Regards, Laurent Pinchart
Re: [PATCH] drm: rcar-du: Re-update the DSYSR register value for start/stop
Hello Hoan,
Thank you for the patch.
On Monday, 22 October 2018 09:30:54 EEST Nguyen An Hoan wrote:
> From: Hoan Nguyen An
>
> From previous commit 0521ccb "drm: rcar-du: Cache DSYSR value to ensure
> known initial value"
What exact commit are you referring to ? The mainline commit that has this
subject is 9144adc5e5a99577bce0d4ee2ca3615f53b9d296.
> We only need to update DSYSR0, DSYSR2 for start/stop.
> So using rgrp-> mmio_offset is enough, the change back from rcar_du_crtc ->
> rcar_du_group -> rcar_du_crtc leading to mmio addresses for DSYSR may be
> different.
Is this fixing an actual problem ? If you look at the code, the line
struct rcar_du_crtc *rcrtc = &rgrp->dev->crtcs[rgrp->index * 2];
makes sure that we select DU0 or DU2 only, so the register write
rcar_du_crtc_dsysr_clr_set(rcrtc, DSYSR_DRES | DSYSR_DEN,
start ? DSYSR_DEN : DSYSR_DRES);
should only access DSYSR0 and DSYSR2.
> Signed-off-by: Hoan Nguyen An
> ---
> drivers/gpu/drm/rcar-du/rcar_du_group.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> b/drivers/gpu/drm/rcar-du/rcar_du_group.c index d85f0a1..a5f7eed 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> @@ -202,10 +202,9 @@ void rcar_du_group_put(struct rcar_du_group *rgrp)
>
> static void __rcar_du_group_start_stop(struct rcar_du_group *rgrp, bool
> start) {
> - struct rcar_du_crtc *rcrtc = &rgrp->dev->crtcs[rgrp->index * 2];
> -
> - rcar_du_crtc_dsysr_clr_set(rcrtc, DSYSR_DRES | DSYSR_DEN,
> -start ? DSYSR_DEN : DSYSR_DRES);
> + rcar_du_group_write(rgrp, DSYSR,
> + (rcar_du_group_read(rgrp, DSYSR) & ~(DSYSR_DRES |
> DSYSR_DEN)) |
> + (start ? DSYSR_DEN : DSYSR_DRES));
> }
>
> void rcar_du_group_start_stop(struct rcar_du_group *rgrp, bool start)
--
Regards,
Laurent Pinchart
Re: HDMI doesn't work on ULCB board
Hi Morimoto-san, On Thursday, 18 October 2018 05:12:14 EEST Kuninori Morimoto wrote: > Hi Vladimir, Laurent > > I'm testing ULCB board, and I noticed that HDMI doen't work on it. Which ULCB board is this ? > - Sometimes kernel panic happen (I lost log, but it said "get crc" > something error), - Often, kernel doesn't boot with below log if I > connected HDMI cable when booting > - Monitor say "No signal" if I connected cable *after* booting > > I'm using Geert's renesas-drivers-2018-10-09-v4.19-rc7 branch. > It seems salvator HDMI is working correctly on it. > I attached my .config > > -- kernel log - > ... > [1.728010] renesas_irqc e61c.interrupt-controller: driving 6 irqs > [1.739166] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). > [1.745851] [drm] No driver support for vblank timestamp query. > [1.787533] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). > [1.794226] [drm] No driver support for vblank timestamp query. > [1.821091] rcar-dmac e670.dma-controller: ignoring dependency for > device, assuming no driver > [1.835807] rcar-dmac e730.dma-controller: ignoring dependency for > device, assuming no driver > [1.850397] rcar-dmac e731.dma-controller: ignoring dependency for > device, assuming no driver > [1.864799] rcar-dmac ec70.dma-controller: ignoring dependency for > device, assuming no driver > [1.879168] rcar-dmac ec72.dma-controller: ignoring dependency for > device, assuming no driver > [1.894102] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). > [1.900788] [drm] No driver support for vblank timestamp query. > [1.907851] ravb e680.ethernet: ignoring dependency for device, > assuming no driver > [1.917038] libphy: ravb_mii: probed > [1.922342] ravb e680.ethernet eth0: Base address at 0xe680, > 2e:09:0a:00:83:2b, IRQ 116. > [1.931494] rcar-fcp fe95.fcp: ignoring dependency for device, > assuming no driver > [1.939736] rcar-fcp fe951000.fcp: ignoring dependency for device, > assuming no driver > [1.947894] rcar-fcp fe96f000.fcp: ignoring dependency for device, > assuming no driver > [1.956044] rcar-fcp fe92f000.fcp: ignoring dependency for device, > assuming no driver > [1.964206] rcar-fcp fe9af000.fcp: ignoring dependency for device, > assuming no driver > [1.972359] rcar-fcp fe9bf000.fcp: ignoring dependency for device, > assuming no driver > [1.980517] rcar-fcp fea27000.fcp: ignoring dependency for device, > assuming no driver > [1.988669] rcar-fcp fea2f000.fcp: ignoring dependency for device, > assuming no driver > [1.996825] rcar-fcp fea37000.fcp: ignoring dependency for device, > assuming no driver > [2.040729] rcar_sound ec50.sound: probed > [2.045763] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). > [2.052441] [drm] No driver support for vblank timestamp query. > /* here kernel stop */ I will have a look. -- Regards, Laurent Pinchart
[PATCH 1/3] dt-bindings: display: renesas: lvds: Document r8a77965 bindings
The M3-N (r8a77965) includes one LVDS encoder. Extend the binding to support it. Signed-off-by: Laurent Pinchart --- .../devicetree/bindings/display/bridge/renesas,lvds.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt b/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt index 3aeb0ec06fd0..ba5469dd09f3 100644 --- a/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt +++ b/Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt @@ -13,6 +13,7 @@ Required properties: - "renesas,r8a7793-lvds" for R8A7793 (R-Car M2-N) compatible LVDS encoders - "renesas,r8a7795-lvds" for R8A7795 (R-Car H3) compatible LVDS encoders - "renesas,r8a7796-lvds" for R8A7796 (R-Car M3-W) compatible LVDS encoders + - "renesas,r8a77965-lvds" for R8A77965 (R-Car M3-N) compatible LVDS encoders - "renesas,r8a77970-lvds" for R8A77970 (R-Car V3M) compatible LVDS encoders - "renesas,r8a77980-lvds" for R8A77980 (R-Car V3H) compatible LVDS encoders - "renesas,r8a77990-lvds" for R8A77990 (R-Car E3) compatible LVDS encoders -- Regards, Laurent Pinchart
[PATCH 0/3] R-Car LVDS: Add R8A77965 (M3-N) support
Hello, This small patch series adds support for the M3-N internal LVDS encoder in the DT bindings, the LVDS encoder driver and the M3-N .dtsi. Please see individual patches for details. The series is based on a merge of Simon's devel branch and the DRM -next branch. Laurent Pinchart (2): dt-bindings: display: renesas: lvds: Document r8a77965 bindings drm: rcar-du: lvds: Add R8A77965 support arm64: dts: renesas: r8a77965: Add LVDS support arch/arm64/boot/dts/renesas/r8a77965.dtsi | 27 .../devicetree/bindings/display/bridge/renesas,lvds.txt | 1 + drivers/gpu/drm/rcar-du/rcar_lvds.c | 1 + 3 files changed, 29 insertions(+) -- Regards, Laurent Pinchart
[PATCH 2/3] drm: rcar-du: lvds: Add R8A77965 support
Add support for the R-Car M3-N (R8A77965) SoC to the LVDS encoder
driver. The encoder appears identical to the M3-W version, we can thus
simply point to the generic Gen3 data.
Signed-off-by: Laurent Pinchart
---
drivers/gpu/drm/rcar-du/rcar_lvds.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c
b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 173d7ad0b991..534a128a869d 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -790,6 +790,7 @@ static const struct of_device_id rcar_lvds_of_table[] = {
{ .compatible = "renesas,r8a7793-lvds", .data = &rcar_lvds_gen2_info },
{ .compatible = "renesas,r8a7795-lvds", .data = &rcar_lvds_gen3_info },
{ .compatible = "renesas,r8a7796-lvds", .data = &rcar_lvds_gen3_info },
+ { .compatible = "renesas,r8a77965-lvds", .data = &rcar_lvds_gen3_info },
{ .compatible = "renesas,r8a77970-lvds", .data =
&rcar_lvds_r8a77970_info },
{ .compatible = "renesas,r8a77980-lvds", .data = &rcar_lvds_gen3_info },
{ .compatible = "renesas,r8a77990-lvds", .data =
&rcar_lvds_r8a77990_info },
--
Regards,
Laurent Pinchart
[PATCH 3/3] arm64: dts: renesas: r8a77965: Add LVDS support
The M3-N (r8a77965) platform has oneLVDS encoder connected to the DU.
Add the corresponding DT node and wire it up.
Signed-off-by: Laurent Pinchart
---
arch/arm64/boot/dts/renesas/r8a77965.dtsi | 27 +++
1 file changed, 27 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/r8a77965.dtsi
b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
index 83946ca2eba5..b984b85dc066 100644
--- a/arch/arm64/boot/dts/renesas/r8a77965.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
@@ -2153,6 +2153,33 @@
port@2 {
reg = <2>;
du_out_lvds0: endpoint {
+ remote-endpoint = <&lvds0_in>;
+ };
+ };
+ };
+ };
+
+ lvds0: lvds@feb9 {
+ compatible = "renesas,r8a77965-lvds";
+ reg = <0 0xfeb9 0 0x14>;
+ clocks = <&cpg CPG_MOD 727>;
+ power-domains = <&sysc R8A77965_PD_ALWAYS_ON>;
+ resets = <&cpg 727>;
+ status = "disabled";
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ lvds0_in: endpoint {
+ remote-endpoint =
<&du_out_lvds0>;
+ };
+ };
+ port@1 {
+ reg = <1>;
+ lvds0_out: endpoint {
};
};
};
--
Regards,
Laurent Pinchart
[PATCH] drm: rcar-du: Fix vblank initialization
The drm_vblank_init() takes the total number of CRTCs as an argument,
but the rcar-du driver passes a bitmask of the CRTC indices. Fix it.
Fixes: 4bf8e1962f91 ("drm: Renesas R-Car Display Unit DRM driver")
Reported-by: Tomi Valkeinen
Signed-off-by: Laurent Pinchart
---
drivers/gpu/drm/rcar-du/rcar_du_kms.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index 4ebd61ecbee1..fe6f65c94eef 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -582,7 +582,7 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
* Initialize vertical blanking interrupts handling. Start with vblank
* disabled for all CRTCs.
*/
- ret = drm_vblank_init(dev, (1 << rcdu->num_crtcs) - 1);
+ ret = drm_vblank_init(dev, rcdu->num_crtcs);
if (ret < 0)
return ret;
--
Regards,
Laurent Pinchart
Re: [PATCH] ARM: dts: r8a779[01]: Disable unconnected LVDS encoders
Hello,
On Wednesday, 17 October 2018 20:48:01 EEST Laurent Pinchart wrote:
> The LVDS0 encoder on Koelsh and Porter, and the LVDS1 encoder on Lager,
> are enabled in DT but have no device connected to their output. This
> result in spurious messages being printed to the kernel log such as
>
> rcar-du feb0.display: no connector for encoder /soc/lvds@feb9,
> skipping
>
> Fix it by disabling the encoders.
>
> Fixes: 15a1ff30d8f9 ("ARM: dts: r8a7790: Convert to new LVDS DT bindings")
> Fixes: e5c3f4707f39 ("ARM: dts: r8a7791: Convert to new LVDS DT bindings")
> Reported-by: Simon Horman
This should have read
Reported-by: Geert Uytterhoeven
Sorry for the mistake, I had read the e-mail thread containing the report
incorrectly. Simon, could you fix this when applying to your tree, or should I
send a v2 ?
> Signed-off-by: Laurent Pinchart
> ---
> arch/arm/boot/dts/r8a7790-lager.dts | 2 --
> arch/arm/boot/dts/r8a7791-koelsch.dts | 2 --
> arch/arm/boot/dts/r8a7791-porter.dts | 2 --
> 3 files changed, 6 deletions(-)
>
> diff --git a/arch/arm/boot/dts/r8a7790-lager.dts
> b/arch/arm/boot/dts/r8a7790-lager.dts index 50312e752e2f..7b9508e83d46
> 100644
> --- a/arch/arm/boot/dts/r8a7790-lager.dts
> +++ b/arch/arm/boot/dts/r8a7790-lager.dts
> @@ -489,8 +489,6 @@
> };
>
> &lvds1 {
> - status = "okay";
> -
> ports {
> port@1 {
> lvds_connector: endpoint {
> diff --git a/arch/arm/boot/dts/r8a7791-koelsch.dts
> b/arch/arm/boot/dts/r8a7791-koelsch.dts index ce22db01fbba..e6580aa0cea3
> 100644
> --- a/arch/arm/boot/dts/r8a7791-koelsch.dts
> +++ b/arch/arm/boot/dts/r8a7791-koelsch.dts
> @@ -479,8 +479,6 @@
> };
>
> &lvds0 {
> - status = "okay";
> -
> ports {
> port@1 {
> lvds_connector: endpoint {
> diff --git a/arch/arm/boot/dts/r8a7791-porter.dts
> b/arch/arm/boot/dts/r8a7791-porter.dts index f02036e5de01..fefdf8238bbe
> 100644
> --- a/arch/arm/boot/dts/r8a7791-porter.dts
> +++ b/arch/arm/boot/dts/r8a7791-porter.dts
> @@ -482,8 +482,6 @@
> };
>
> &lvds0 {
> - status = "okay";
> -
> ports {
> port@1 {
> lvds_connector: endpoint {
--
Regards,
Laurent Pinchart
[PATCH] ARM: dts: r8a779[01]: Disable unconnected LVDS encoders
The LVDS0 encoder on Koelsh and Porter, and the LVDS1 encoder on Lager,
are enabled in DT but have no device connected to their output. This
result in spurious messages being printed to the kernel log such as
rcar-du feb0.display: no connector for encoder /soc/lvds@feb9, skipping
Fix it by disabling the encoders.
Fixes: 15a1ff30d8f9 ("ARM: dts: r8a7790: Convert to new LVDS DT bindings")
Fixes: e5c3f4707f39 ("ARM: dts: r8a7791: Convert to new LVDS DT bindings")
Reported-by: Simon Horman
Signed-off-by: Laurent Pinchart
---
arch/arm/boot/dts/r8a7790-lager.dts | 2 --
arch/arm/boot/dts/r8a7791-koelsch.dts | 2 --
arch/arm/boot/dts/r8a7791-porter.dts | 2 --
3 files changed, 6 deletions(-)
diff --git a/arch/arm/boot/dts/r8a7790-lager.dts
b/arch/arm/boot/dts/r8a7790-lager.dts
index 50312e752e2f..7b9508e83d46 100644
--- a/arch/arm/boot/dts/r8a7790-lager.dts
+++ b/arch/arm/boot/dts/r8a7790-lager.dts
@@ -489,8 +489,6 @@
};
&lvds1 {
- status = "okay";
-
ports {
port@1 {
lvds_connector: endpoint {
diff --git a/arch/arm/boot/dts/r8a7791-koelsch.dts
b/arch/arm/boot/dts/r8a7791-koelsch.dts
index ce22db01fbba..e6580aa0cea3 100644
--- a/arch/arm/boot/dts/r8a7791-koelsch.dts
+++ b/arch/arm/boot/dts/r8a7791-koelsch.dts
@@ -479,8 +479,6 @@
};
&lvds0 {
- status = "okay";
-
ports {
port@1 {
lvds_connector: endpoint {
diff --git a/arch/arm/boot/dts/r8a7791-porter.dts
b/arch/arm/boot/dts/r8a7791-porter.dts
index f02036e5de01..fefdf8238bbe 100644
--- a/arch/arm/boot/dts/r8a7791-porter.dts
+++ b/arch/arm/boot/dts/r8a7791-porter.dts
@@ -482,8 +482,6 @@
};
&lvds0 {
- status = "okay";
-
ports {
port@1 {
lvds_connector: endpoint {
--
Regards,
Laurent Pinchart
Re: [PATCH v2 3/5] drm: rcar-du: Add r8a77470 support
Hi Fabrizio,
Thank you for the patch.
On Tuesday, 16 October 2018 19:58:59 EEST Fabrizio Castro wrote:
> Add RZ/G1C (a.k.a. r8a77470) support to the R-Car DU driver.
>
> Signed-off-by: Fabrizio Castro
> Reviewed-by: Laurent Pinchart
>
> ---
> v1->v2:
> * Added flags RCAR_DU_FEATURE_INTERLACED and RCAR_DU_FEATURE_TVM_SYNC
> * Reworked comment
This looks all good, applied to my tree.
> drivers/gpu/drm/rcar-du/rcar_du_drv.c | 28
> 1 file changed, 28 insertions(+)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 084f58d..d8a02c4 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -77,6 +77,33 @@ static const struct rcar_du_device_info
> rzg1_du_r8a7745_info = { },
> };
>
> +static const struct rcar_du_device_info rzg1_du_r8a77470_info = {
> + .gen = 2,
> + .features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
> + | RCAR_DU_FEATURE_EXT_CTRL_REGS
> + | RCAR_DU_FEATURE_INTERLACED
> + | RCAR_DU_FEATURE_TVM_SYNC,
> + .channels_mask = BIT(1) | BIT(0),
> + .routes = {
> + /*
> + * R8A77470 has two RGB outputs, one LVDS output, and
> + * one (currently unsupported) analog video output
> + */
> + [RCAR_DU_OUTPUT_DPAD0] = {
> + .possible_crtcs = BIT(0),
> + .port = 0,
> + },
> + [RCAR_DU_OUTPUT_DPAD1] = {
> + .possible_crtcs = BIT(1),
> + .port = 1,
> + },
> + [RCAR_DU_OUTPUT_LVDS0] = {
> + .possible_crtcs = BIT(0) | BIT(1),
> + .port = 2,
> + },
> + },
> +};
> +
> static const struct rcar_du_device_info rcar_du_r8a7779_info = {
> .gen = 2,
> .features = RCAR_DU_FEATURE_INTERLACED
> @@ -342,6 +369,7 @@ static const struct rcar_du_device_info
> rcar_du_r8a7799x_info = { static const struct of_device_id
> rcar_du_of_table[] = {
> { .compatible = "renesas,du-r8a7743", .data = &rzg1_du_r8a7743_info },
> { .compatible = "renesas,du-r8a7745", .data = &rzg1_du_r8a7745_info },
> + { .compatible = "renesas,du-r8a77470", .data = &rzg1_du_r8a77470_info },
> { .compatible = "renesas,du-r8a7779", .data = &rcar_du_r8a7779_info },
> { .compatible = "renesas,du-r8a7790", .data = &rcar_du_r8a7790_info },
> { .compatible = "renesas,du-r8a7791", .data = &rcar_du_r8a7791_info },
--
Regards,
Laurent Pinchart
[PATCH] MAINTAINERS: Remove Laurent Pinchart as Renesas pinctrl maintainer
Geert Uytterhoeven has long taken over and I'm not involved anymore with the Renesas pinctrl driver. Remove myself from the maintainers list. Signed-off-by: Laurent Pinchart --- MAINTAINERS | 1 - 1 file changed, 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 69373eb328d4..f9d00cc05202 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11515,7 +11515,6 @@ F: Documentation/devicetree/bindings/pinctrl/qcom,*.txt F: drivers/pinctrl/qcom/ PIN CONTROLLER - RENESAS -M: Laurent Pinchart M: Geert Uytterhoeven L: linux-renesas-soc@vger.kernel.org T: git git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git sh-pfc -- Regards, Laurent Pinchart
Re: [PATCH] drm: rcar-du: Fix the return value in case of error in 'rcar_du_crtc_set_crc_source()'
Hi Christophe,
Thank you for the patch.
On Tuesday, 16 October 2018 09:58:25 EEST Christophe JAILLET wrote:
> We return 0 unconditionally in 'rcar_du_crtc_set_crc_source()'.
> However, 'ret' is set to some error codes if some function calls fail.
>
> Return 'ret' instead to propagate the error code.
>
> Fixes: 47a52d024e89 ("media: drm: rcar-du: Add support for CRC computation")
> Signed-off-by: Christophe JAILLET
Reviewed-by: Laurent Pinchart
and applied to my tree.
> ---
> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 8a9e5e6f16b4..6eb0a95ad5a6
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -940,7 +940,7 @@ static int rcar_du_crtc_set_crc_source(struct drm_crtc
> *crtc, drm_modeset_drop_locks(&ctx);
> drm_modeset_acquire_fini(&ctx);
>
> - return 0;
> + return ret;
> }
>
> static const struct drm_crtc_funcs crtc_funcs_gen2 = {
--
Regards,
Laurent Pinchart
Re: [PATCH v3 1/4] dt-bindings: media: i2c: Add bindings for Maxim Integrated MAX9286
Hello, On Monday, 15 October 2018 22:01:21 EEST Niklas Söderlund wrote: > On 2018-10-15 18:37:40 +0100, Kieran Bingham wrote: > >>> diff --git > >>> a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.txt > >>> b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.txt > >>> new file mode 100644 > >>> index ..a73e3c0dc31b > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.txt > >>> @@ -0,0 +1,182 @@ > >>> +Maxim Integrated Quad GMSL Deserializer > >>> +--- > >>> + > >>> +The MAX9286 deserializer receives video data on up to 4 Gigabit > >>> Multimedia > >>> +Serial Links (GMSL) and outputs them on a CSI-2 port using up to 4 > >>> data lanes. > >> > >> CSI-2 D-PHY I presume? > > > > Yes, that's how I've adapted the driver based on the latest bus changes. > > > > Niklas - Could you confirm that everything in VIN/CSI2 is configured to > > use D-PHY and not C-PHY at all ? > > Yes it's only D-PHY. > > >>> + > >>> +- remote-endpoint: phandle to the remote GMSL source endpoint subnode > >>> in the > >>> + remote node port. > >>> + > >>> +Required Endpoint Properties for CSI-2 Output Port (Port 4): > >>> + > >>> +- data-lanes: array of physical CSI-2 data lane indexes. > >>> +- clock-lanes: index of CSI-2 clock lane. > >> > >> Is any number of lanes supported? How about lane remapping? If you do > >> not have lane remapping, the clock-lanes property is redundant. > > > > Uhm ... Niklas? > > The MAX9286 documentation contains information on lane remapping and > support for any number (1-4) of enabled data-lanes. I have not tested if > this works in practice but the registers are there and documented :-) That's my understanding too. Clock lane remapping doesn't seem to be supported though. We could thus omit the clock-lanes property. -- Regards, Laurent Pinchart
Re: [PATCH 4/5] drm: rcar-du: Add R8A7744 support
Hi Fabrizio,
Thank you for the patch.
On Friday, 21 September 2018 21:08:30 EEST Fabrizio Castro wrote:
> From: Biju Das
>
> Add support for the R8A7744 DU (which is very similar to the R8A7743 DU);
> it has 1 DPAD (RGB) output and 1 LVDS output.
>
> Signed-off-by: Biju Das
> Reviewed-by: Fabrizio Castro
> ---
> drivers/gpu/drm/rcar-du/rcar_du_drv.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index c07d3f1..2c3d0e5 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -321,6 +321,7 @@ static const struct rcar_du_device_info
> rcar_du_r8a77970_info = {
>
> static const struct of_device_id rcar_du_of_table[] = {
> { .compatible = "renesas,du-r8a7743", .data = &rzg1_du_r8a7743_info },
> + { .compatible = "renesas,du-r8a7744", .data = &rzg1_du_r8a7743_info },
> { .compatible = "renesas,du-r8a7745", .data = &rzg1_du_r8a7745_info },
> { .compatible = "renesas,du-r8a77470", .data = &rzg1_du_r8a77470_info },
> { .compatible = "renesas,du-r8a7779", .data = &rcar_du_r8a7779_info },
This looks good to me. I would also apply this change:
@@ -41,7 +41,7 @@ static const struct rcar_du_device_info rzg1_du_r8a7743_info
= {
.channels_mask = BIT(1) | BIT(0),
.routes = {
/*
-* R8A7743 has one RGB output and one LVDS output
+* R8A774[34] has one RGB output and one LVDS output
*/
[RCAR_DU_OUTPUT_DPAD0] = {
.possible_crtcs = BIT(1) | BIT(0),
With this,
Reviewed-by: Laurent Pinchart
There's no need to resubmit, I've applied the patch to my tree with the above
change.
--
Regards,
Laurent Pinchart
Re: [PATCH 3/5] drm: rcar-du: Add r8a77470 support
Hi Fabrizio,
Thank you for the patch.
On Friday, 21 September 2018 21:08:29 EEST Fabrizio Castro wrote:
> Add RZ/G1C (a.k.a. r8a77470) support to the R-Car DU driver.
>
> Signed-off-by: Fabrizio Castro
> Reviewed-by: Biju Das
> ---
> drivers/gpu/drm/rcar-du/rcar_du_drv.c | 26 ++
> 1 file changed, 26 insertions(+)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 02aee6c..c07d3f1 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -77,6 +77,31 @@ static const struct rcar_du_device_info
> rzg1_du_r8a7745_info = { },
> };
>
> +static const struct rcar_du_device_info rzg1_du_r8a77470_info = {
> + .gen = 2,
> + .features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
> + | RCAR_DU_FEATURE_EXT_CTRL_REGS,
The DU driver has recently gained two new feature flags,
RCAR_DU_FEATURE_INTERLACED and RCAR_DU_FEATURE_TVM_SYNC. I believe that both
should be set here. Could you please confirm (and test) ?
> + .channels_mask = BIT(1) | BIT(0),
> + .routes = {
> + /*
> + * R8A77470 has two RGB outputs, one LVDS output, and
> + * one analog video output (unsupported)
I'd write this "and one (currently unsupported) analog video output" to match
the other device entries. With this fixed,
Reviewed-by: Laurent Pinchart
> + */
> + [RCAR_DU_OUTPUT_DPAD0] = {
> + .possible_crtcs = BIT(0),
> + .port = 0,
> + },
> + [RCAR_DU_OUTPUT_DPAD1] = {
> + .possible_crtcs = BIT(1),
> + .port = 1,
> + },
> + [RCAR_DU_OUTPUT_LVDS0] = {
> + .possible_crtcs = BIT(0) | BIT(1),
> + .port = 2,
> + },
> + },
> +};
> +
> static const struct rcar_du_device_info rcar_du_r8a7779_info = {
> .gen = 2,
> .features = 0,
> @@ -297,6 +322,7 @@ static const struct rcar_du_device_info
> rcar_du_r8a77970_info = { static const struct of_device_id
> rcar_du_of_table[] = {
> { .compatible = "renesas,du-r8a7743", .data = &rzg1_du_r8a7743_info },
> { .compatible = "renesas,du-r8a7745", .data = &rzg1_du_r8a7745_info },
> + { .compatible = "renesas,du-r8a77470", .data = &rzg1_du_r8a77470_info },
> { .compatible = "renesas,du-r8a7779", .data = &rcar_du_r8a7779_info },
> { .compatible = "renesas,du-r8a7790", .data = &rcar_du_r8a7790_info },
> { .compatible = "renesas,du-r8a7791", .data = &rcar_du_r8a7791_info },
--
Regards,
Laurent Pinchart
Re: [PATCH 2/5] dt-bindings: display: renesas: du: Document the r8a7744 bindings
Hi Fabrizio, Thank you for the patch. On Friday, 21 September 2018 21:08:28 EEST Fabrizio Castro wrote: > From: Biju Das > > Document the RZ/G1N (R8A7744) SoC in the R-Car DU bindings. > > Signed-off-by: Biju Das > Reviewed-by: Fabrizio Castro Reviewed-by: Laurent Pinchart and taken in my tree. > --- > Documentation/devicetree/bindings/display/renesas,du.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt > b/Documentation/devicetree/bindings/display/renesas,du.txt index > 2b28ae2..aad9cf1 100644 > --- a/Documentation/devicetree/bindings/display/renesas,du.txt > +++ b/Documentation/devicetree/bindings/display/renesas,du.txt > @@ -4,6 +4,7 @@ Required Properties: > >- compatible: must be one of the following. > - "renesas,du-r8a7743" for R8A7743 (RZ/G1M) compatible DU > +- "renesas,du-r8a7744" for R8A7744 (RZ/G1N) compatible DU > - "renesas,du-r8a7745" for R8A7745 (RZ/G1E) compatible DU > - "renesas,du-r8a77470" for R8A77470 (RZ/G1C) compatible DU > - "renesas,du-r8a7779" for R8A7779 (R-Car H1) compatible DU > @@ -51,6 +52,7 @@ corresponding to each DU output. > Port0 Port1 Port2 Port3 > --- > -- R8A7743 (RZ/G1M) DPAD 0 LVDS 0 - - + > R8A7744 (RZ/G1N) DPAD 0 LVDS 0 - - > R8A7745 (RZ/G1E) DPAD 0 DPAD 1 - - > R8A77470 (RZ/G1C) DPAD 0 DPAD 1 LVDS 0 - > R8A7779 (R-Car H1) DPAD 0 DPAD 1 - - -- Regards, Laurent Pinchart
Re: [PATCH 1/5] dt-bindings: display: renesas: du: Document the r8a77470 bindings
Hi Fabrizio, Thank you for the patch. On Friday, 21 September 2018 21:08:27 EEST Fabrizio Castro wrote: > Document the RZ/G1C (r8a77470) SoC in R-Car DU bindings. > > Signed-off-by: Fabrizio Castro > Reviewed-by: Biju Das Reviewed-by: Laurent Pinchart and taken in my tree. > --- > Documentation/devicetree/bindings/display/renesas,du.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt > b/Documentation/devicetree/bindings/display/renesas,du.txt index > ec9d34be..2b28ae2 100644 > --- a/Documentation/devicetree/bindings/display/renesas,du.txt > +++ b/Documentation/devicetree/bindings/display/renesas,du.txt > @@ -5,6 +5,7 @@ Required Properties: >- compatible: must be one of the following. > - "renesas,du-r8a7743" for R8A7743 (RZ/G1M) compatible DU > - "renesas,du-r8a7745" for R8A7745 (RZ/G1E) compatible DU > +- "renesas,du-r8a77470" for R8A77470 (RZ/G1C) compatible DU > - "renesas,du-r8a7779" for R8A7779 (R-Car H1) compatible DU > - "renesas,du-r8a7790" for R8A7790 (R-Car H2) compatible DU > - "renesas,du-r8a7791" for R8A7791 (R-Car M2-W) compatible DU > @@ -51,6 +52,7 @@ corresponding to each DU output. > --- > -- R8A7743 (RZ/G1M) DPAD 0 LVDS 0 - - > R8A7745 (RZ/G1E) DPAD 0 DPAD 1 - - + > R8A77470 (RZ/G1C) DPAD 0 DPAD 1 LVDS 0 - > R8A7779 (R-Car H1) DPAD 0 DPAD 1 - - > R8A7790 (R-Car H2) DPAD 0 LVDS 0 LVDS 1 - > R8A7791 (R-Car M2-W) DPAD 0 LVDS 0 - - -- Regards, Laurent Pinchart
Re: [PATCH v2 1/5] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints
Hi Jacopo, On Friday, 5 October 2018 11:49:45 EEST jacopo mondi wrote: > On Fri, Oct 05, 2018 at 01:00:47AM +0300, Laurent Pinchart wrote: > > On Friday, 5 October 2018 00:42:17 EEST Laurent Pinchart wrote: > >> On Thursday, 4 October 2018 23:41:34 EEST Niklas Söderlund wrote: > >>> From: Niklas Söderlund > >>> > >>> The CSI-2 transmitters can use a different number of lanes to transmit > >>> data. Make the data-lanes mandatory for the endpoints describe the > >> > >> s/describe/that describe/ ? > >> > >>> transmitters as no good default can be set to fallback on. > >>> > >>> Signed-off-by: Niklas Söderlund > >>> > >>> --- > >>> > >>> Documentation/devicetree/bindings/media/i2c/adv748x.txt | 3 +++ > >>> 1 file changed, 3 insertions(+) > >>> > >>> diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt > >>> b/Documentation/devicetree/bindings/media/i2c/adv748x.txt index > >>> 5dddc95f9cc46084..f9dac01ab795fc28 100644 > >>> --- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt > >>> +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt > >>> @@ -50,6 +50,9 @@ are numbered as follows. > >>> > >>> The digital output port nodes must contain at least one endpoint. > >>> > >>> +The endpoints described in TXA and TXB ports must if present contain > >>> +the data-lanes property as described in video-interfaces.txt. > >>> + > >> > >> Would it make sense to merge those two paragraphs, as they refer to the > >> same endpoint ? > >> > >> "The digital output port nodes, when present, shall contain at least one > >> endpoint. Each of those endpoints shall contain the data-lanes property > >> as described in video-interfaces.txt." > >> > >> (DT bindings normally use "shall" instead of "must", but that hasn't > >> really been enforced.) > >> > >> If you want to keep the paragraphs separate, I would recommend using > >> "digital output ports" instead of "TXA and TXB" in the second paragraph > >> for consistency (or the other way around). > >> > >> I'm fine with any of the above option, so please pick your favourite, > >> and add > >> > >> Reviewed-by: Laurent Pinchart > > > > I just realized that TXB only supports a single data lane, so we may want > > not to have a data-lanes property for TXB. > > Isn't it better to restrict its value to <1> but make it mandatory > anyhow? I understand conceptually that property should not be there, > as it has a single acceptable value, but otherwise we need to traeat > differently the two output ports, in documentation and code. The two ports are different, so I wouldn't be shocked if we handled them differently :-) I believe it would actually reduce the code size (and save CPU cycles at runtime). > Why not inserting a paragraph with the required endpoint properties > description? > > Eg: > > Required endpoint properties: > - data-lanes: See "video-interfaces.txt" for description. The property >is mandatory for CSI-2 output endpoints and the accepted values >depends on which endpoint the property is applied to: >- TXA: accepted values are <1>, <2>, <4> >- TXB: accepted value is <1> > > >>> Ports are optional if they are not connected to anything at the > >>> hardware level. > >>> > >>> Example: -- Regards, Laurent Pinchart
Re: [PATCH v2 2/5] i2c: adv748x: reorder register writes for CSI-2 transmitters initialization
Hi Niklas,
Thank you for the patch.
On Thursday, 4 October 2018 23:41:35 EEST Niklas Söderlund wrote:
> From: Niklas Söderlund
>
> Reorder the initialization order of registers to allow for refactoring.
> The move could have been done at the same time as the refactoring but
> since the documentation about some registers involved are missing do it
> separately.
>
> Signed-off-by: Niklas Söderlund
> ---
> drivers/media/i2c/adv748x/adv748x-core.c | 12 +++-
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> b/drivers/media/i2c/adv748x/adv748x-core.c index
> 6854d898fdd1f192..721ed6552bc1cde6 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -383,8 +383,6 @@ static const struct adv748x_reg_value
> adv748x_init_txa_4lane[] = { {ADV748X_PAGE_IO, 0x0c, 0xe0}, /* Enable
> LLC_DLL & Double LLC Timing */ {ADV748X_PAGE_IO, 0x0e, 0xdd}, /*
> LLC/PIX/SPI PINS TRISTATED AUD */
>
> - {ADV748X_PAGE_TXA, 0x00, 0x84}, /* Enable 4-lane MIPI */
> - {ADV748X_PAGE_TXA, 0x00, 0xa4}, /* Set Auto DPHY Timing */
> {ADV748X_PAGE_TXA, 0xdb, 0x10}, /* ADI Required Write */
> {ADV748X_PAGE_TXA, 0xd6, 0x07}, /* ADI Required Write */
> {ADV748X_PAGE_TXA, 0xc4, 0x0a}, /* ADI Required Write */
> @@ -392,6 +390,9 @@ static const struct adv748x_reg_value
> adv748x_init_txa_4lane[] = { {ADV748X_PAGE_TXA, 0x72, 0x11}, /* ADI
> Required Write */
> {ADV748X_PAGE_TXA, 0xf0, 0x00}, /* i2c_dphy_pwdn - 1'b0 */
>
> + {ADV748X_PAGE_TXA, 0x00, 0x84}, /* Enable 4-lane MIPI */
> + {ADV748X_PAGE_TXA, 0x00, 0xa4}, /* Set Auto DPHY Timing */
> +
> {ADV748X_PAGE_TXA, 0x31, 0x82}, /* ADI Required Write */
> {ADV748X_PAGE_TXA, 0x1e, 0x40}, /* ADI Required Write */
> {ADV748X_PAGE_TXA, 0xda, 0x01}, /* i2c_mipi_pll_en - 1'b1 */
> @@ -435,17 +436,18 @@ static const struct adv748x_reg_value
> adv748x_init_txb_1lane[] = { {ADV748X_PAGE_SDP, 0x31, 0x12}, /* ADI
> Required Write */
> {ADV748X_PAGE_SDP, 0xe6, 0x4f}, /* V bit end pos manually in NTSC */
>
> - {ADV748X_PAGE_TXB, 0x00, 0x81}, /* Enable 1-lane MIPI */
> - {ADV748X_PAGE_TXB, 0x00, 0xa1}, /* Set Auto DPHY Timing */
> {ADV748X_PAGE_TXB, 0xd2, 0x40}, /* ADI Required Write */
> {ADV748X_PAGE_TXB, 0xc4, 0x0a}, /* ADI Required Write */
> {ADV748X_PAGE_TXB, 0x71, 0x33}, /* ADI Required Write */
> {ADV748X_PAGE_TXB, 0x72, 0x11}, /* ADI Required Write */
> {ADV748X_PAGE_TXB, 0xf0, 0x00}, /* i2c_dphy_pwdn - 1'b0 */
> +
> + {ADV748X_PAGE_TXB, 0x00, 0x81}, /* Enable 1-lane MIPI */
> + {ADV748X_PAGE_TXB, 0x00, 0xa1}, /* Set Auto DPHY Timing */
> +
This is pretty hard to review, as there's a bunch of undocumented register
writes. I think the first write is safe, as the tables are written immediately
following a software reset, and the default value of the register is 0x81
(CSI-TX disabled, 1 lane). The second write, however, enables usage of the
computed DPHY parameters, and I don't know whether the undocumented register
writes in-between may interact with that.
That being said, this change enables further important refactoring, so I'm
tempted to accept it. I assume you've tested it and haven't noticed a
regression. The part that still bothers me in particular is that the write to
register 0xf0 just above this takes the DPHY out of power down according to
the datasheet, and I wonder whether at that point the DPHY might not react to
that information. Have you analyzed the power-up sequence in section 9.5.1 of
the hardware manual ? I wonder whether the dphy_pwdn shouldn't be handled in
the power up and power down sequences, which might involve also moving the
above four (and five for TXA) undocumented writes to the power up sequence as
well.
> {ADV748X_PAGE_TXB, 0x31, 0x82}, /* ADI Required Write */
> {ADV748X_PAGE_TXB, 0x1e, 0x40}, /* ADI Required Write */
> {ADV748X_PAGE_TXB, 0xda, 0x01}, /* i2c_mipi_pll_en - 1'b1 */
> -
> {ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> {ADV748X_PAGE_TXB, 0x00, 0x21 },/* Power-up CSI-TX */
> {ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
--
Regards,
Laurent Pinchart
