Hi Laurent,
On Fri, Mar 08, 2019 at 08:12:39PM +0200, Laurent Pinchart wrote:
> 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.
>
Fine, I tried thinking up a bit if the "renesas,companion" property
could have been made an endpoint property, to be specified in both
the rcar-lvds endpoints and in the DU endpoints, so that the
rcar_du_encoder does not need to pick into the lvds-encoder to know
if it is operating in dual link mode or not and skip creation of the
encoder associated to LVDS1 in such a case.
Furthermore, we could make a "vstripe-even" "vstripe-odd" endpoint
properties, that would allow you to control the ST_SWAP field of
LVDSTRIPE register, and create another endpoint property that contains
the phandle to the companion, like "dual-link-companion" or
"dual-link-slave". Those properties would need to be specified in both
DU and LVDS endpoints though, which might be clunky, but that would
possibly save a few cross-driver calls.
Anyway, just putting a few more options on the table, if you have
already considered those feel free to stick to this implementation...
> > 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.
I see, still right now is of no use. It's fine though, really minor
stuff.
Thanks
j
>
> > > + 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
Attachment:
signature.asc
Description: PGP signature
