Hi Jacopo,
On Fri, Mar 08, 2019 at 06:20:23PM +0100, Jacopo Mondi wrote:
> On Thu, Mar 07, 2019 at 01:23:41AM +0200, Laurent Pinchart wrote:
> > In dual-link mode the LVDS0 encoder transmits even-numbered pixels, and
> > sends odd-numbered pixels to the LVDS1 encoder for transmission on a
> > separate link.
> >
> > To implement support for this mode of operation, determine if the LVDS
> > connection operates in dual-link mode by querying the next device in the
> > pipeline, locate the companion encoder, and control it directly through
> > its bridge operations.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/rcar-du/rcar_lvds.c | 104 ++++++++++++++++++++++++----
> > drivers/gpu/drm/rcar-du/rcar_lvds.h | 5 ++
> > 2 files changed, 96 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > index 5ac92ee15be0..90d33514bb3e 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > @@ -66,6 +66,9 @@ struct rcar_lvds {
> >
> > struct drm_display_mode display_mode;
> > enum rcar_lvds_mode mode;
> > +
> > + struct drm_bridge *companion;
> > + bool dual_link;
> > };
> >
> > #define bridge_to_rcar_lvds(bridge) \
> > @@ -400,11 +403,6 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
> > {
> > struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
> > const struct drm_display_mode *mode = &lvds->display_mode;
> > - /*
> > - * FIXME: We should really retrieve the CRTC through the state, but how
> > - * do we get a state pointer?
> > - */
> > - struct drm_crtc *crtc = lvds->bridge.encoder->crtc;
> > u32 lvdhcr;
> > u32 lvdcr0;
> > int ret;
> > @@ -413,6 +411,10 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
> > if (ret < 0)
> > return;
> >
> > + /* Enable the companion LVDS encoder in dual-link mode. */
> > + if (lvds->dual_link && lvds->companion)
> > + lvds->companion->funcs->enable(lvds->companion);
> > +
> > /*
> > * Hardcode the channels and control signals routing for now.
> > *
> > @@ -435,17 +437,33 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
> > rcar_lvds_write(lvds, LVDCHCR, lvdhcr);
> >
> > if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
> > - /* Disable dual-link mode. */
> > - rcar_lvds_write(lvds, LVDSTRIPE, 0);
> > + /*
> > + * Configure vertical stripe based on the mode of operation of
> > + * the connected device.
> > + */
> > + rcar_lvds_write(lvds, LVDSTRIPE,
> > + lvds->dual_link ? LVDSTRIPE_ST_ON : 0);
> > }
> >
> > - /* PLL clock configuration. */
> > - lvds->info->pll_setup(lvds, mode->clock * 1000);
> > + /*
> > + * PLL clock configuration on all instances but the companion in
> > + * dual-link mode.
> > + */
> > + if (!lvds->dual_link || lvds->companion)
> > + lvds->info->pll_setup(lvds, mode->clock * 1000);
> >
> > /* Set the LVDS mode and select the input. */
> > lvdcr0 = lvds->mode << LVDCR0_LVMD_SHIFT;
> > - if (drm_crtc_index(crtc) == 2)
> > - lvdcr0 |= LVDCR0_DUSEL;
> > +
> > + if (lvds->bridge.encoder) {
> > + /*
> > + * FIXME: We should really retrieve the CRTC through the state,
> > + * but how do we get a state pointer?
> > + */
> > + if (drm_crtc_index(lvds->bridge.encoder->crtc) == 2)
> > + lvdcr0 |= LVDCR0_DUSEL;
> > + }
> > +
> > rcar_lvds_write(lvds, LVDCR0, lvdcr0);
> >
> > /* Turn all the channels on. */
> > @@ -512,6 +530,10 @@ static void rcar_lvds_disable(struct drm_bridge *bridge)
> > rcar_lvds_write(lvds, LVDCR1, 0);
> > rcar_lvds_write(lvds, LVDPLLCR, 0);
> >
> > + /* Disable the companion LVDS encoder in dual-link mode. */
> > + if (lvds->dual_link && lvds->companion)
> > + lvds->companion->funcs->disable(lvds->companion);
> > +
> > clk_disable_unprepare(lvds->clocks.mod);
> > }
> >
> > @@ -628,10 +650,54 @@ static const struct drm_bridge_funcs rcar_lvds_bridge_ops = {
> > .mode_set = rcar_lvds_mode_set,
> > };
> >
> > +bool rcar_lvds_dual_link(struct drm_bridge *bridge)
> > +{
> > + struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
> > +
> > + return lvds->dual_link;
> > +}
> > +EXPORT_SYMBOL_GPL(rcar_lvds_dual_link);
> > +
> > /* -----------------------------------------------------------------------------
> > * Probe & Remove
> > */
> >
> > +static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
> > +{
> > + const struct of_device_id *match;
> > + struct device_node *companion;
> > + struct device *dev = lvds->dev;
> > + int ret = 0;
> > +
> > + /* Locate the companion LVDS encoder for dual-link operation, if any. */
> > + companion = of_parse_phandle(dev->of_node, "renesas,companion", 0);
> > + if (!companion)
> > + return -ENODEV;
> > +
> > + /*
> > + * Sanity check: the companion encoder must have the same compatible
> > + * string.
> > + */
> > + match = of_match_device(dev->driver->of_match_table, dev);
> > + if (!of_device_is_compatible(companion, match->compatible)) {
> > + ret = -ENODEV;
> > + goto done;
> > + }
> > +
> > + lvds->companion = of_drm_find_bridge(companion);
> > + if (!lvds->companion) {
> > + ret = -EPROBE_DEFER;
> > + goto done;
> > + }
> > +
> > + dev_dbg(dev, "Found companion encoder %pOF\n", companion);
> > +
> > +done:
> > + of_node_put(companion);
> > +
> > + return ret;
> > +}
> > +
> > static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> > {
> > struct device_node *local_output = NULL;
> > @@ -682,14 +748,26 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> >
> > if (is_bridge) {
> > lvds->next_bridge = of_drm_find_bridge(remote);
> > - if (!lvds->next_bridge)
> > + if (!lvds->next_bridge) {
> > ret = -EPROBE_DEFER;
> > + goto done;
> > + }
> > +
> > + if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
> > + lvds->dual_link = lvds->next_bridge->timings
> > + ? lvds->next_bridge->timings->dual_link
> > + : false;
>
> I wonder if, in all this patch, you could not use "lvds->companion" in
> place of "lvds->dual_link", and thus drop that from timings. This mean
> "renesas,companion" would only be used when operating in dual link
> mode, which might be less nice (and could actually called differently
> in that case).
>
> Both the THC631024 and the rcar_du-lvds driver would decide
> independently if they operate in dual link mode or not (one counting
> its endpoints, the other inspecting the "renesas,companion" property).
I decided to specify the companion in DT regardless of which operating
mode is used, as it's a property of the LVDS encoder, not of the
operating mode. Furthermore, I can foresee setups where the mode would
be selected dynamically at runtime. Our development boards don't allow
that as they hardcode the mode using DIP switches, but nothing would
prevent the mode selection signals to be connected to GPIOs. I thus
think lvds->companion should point to the companion unconditionally, and
lvds->dual_link should select the operating mode. The dual_link field is
currently set at probe time as I have no need for dynamic configuration
of the mode and no mean of testing it, so I decided not to implement
dynamic switching yet.
> The only place where this might be tricky is the here above
>
> + /*
> + * PLL clock configuration on all instances but the companion in
> + * dual-link mode.
> + */
> + if (!lvds->dual_link || lvds->companion)
> + lvds->info->pll_setup(lvds, mode->clock * 1000);
>
> but that would remove the need for the thc63 bridge to report its
> operating mode in timings...
>
> > } else {
> > lvds->panel = of_drm_find_panel(remote);
> > - if (IS_ERR(lvds->panel))
> > + if (IS_ERR(lvds->panel)) {
> > ret = PTR_ERR(lvds->panel);
> > + goto done;
> > + }
> > }
> >
> > + if (lvds->dual_link)
>
> Note: if (!is_bridge) you would never set lvds->dual_link, so this
> should be moved inside the here above "if (is_bridge)"
I don't yet, but nothing prevents a panel from operating in dual mode,
even if not implemented yet. That's why I've move this check out of the
bridge/panel conditional code.
> > + ret = rcar_lvds_parse_dt_companion(lvds);
> > +
> > done:
> > of_node_put(local_output);
> > of_node_put(remote_input);
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.h b/drivers/gpu/drm/rcar-du/rcar_lvds.h
> > index a709cae1bc32..222ec0e60785 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.h
> > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.h
> > @@ -15,6 +15,7 @@ struct drm_bridge;
> > #if IS_ENABLED(CONFIG_DRM_RCAR_LVDS)
> > int rcar_lvds_clk_enable(struct drm_bridge *bridge, unsigned long freq);
> > void rcar_lvds_clk_disable(struct drm_bridge *bridge);
> > +bool rcar_lvds_dual_link(struct drm_bridge *bridge);
> > #else
> > static inline int rcar_lvds_clk_enable(struct drm_bridge *bridge,
> > unsigned long freq)
> > @@ -22,6 +23,10 @@ static inline int rcar_lvds_clk_enable(struct drm_bridge *bridge,
> > return -ENOSYS;
> > }
> > static inline void rcar_lvds_clk_disable(struct drm_bridge *bridge) { }
> > +static inline bool rcar_lvds_dual_link(struct drm_bridge *bridge)
> > +{
> > + return false;
> > +}
> > #endif /* CONFIG_DRM_RCAR_LVDS */
> >
> > #endif /* __RCAR_LVDS_H__ */
--
Regards,
Laurent Pinchart
