Re: [PATCH] drm/bridge: ti-sn65dsi86: Implement lane reordering + polarity

[Doug Anderson]

Hi

On Mon, May 4, 2020 at 10:44 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>
> Quoting Douglas Anderson (2020-05-04 21:36:31)
> > The ti-sn65dsi86 MIPI DSI to eDP bridge chip supports arbitrary
> > remapping of eDP lanes and also polarity inversion.  Both of these
> > features have been described in the device tree bindings for the
> > device since the beginning but were never implemented in the driver.
> > Implement both of them.
> >
> > Part of this change also allows you to (via the same device tree
> > bindings) specify to use fewer than the max number of DP lanes that
> > the panel reports.  This could be useful if your display supports more
> > lanes but only a few are hooked up on your board.
> >
> > Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> > ---
>
> Except for one thing below:
>
> Reviewed-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
>
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 1a125423eb07..52cca54b525f 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -707,26 +716,20 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
> >         int dp_rate_idx;
> >         unsigned int val;
> >         int ret = -EINVAL;
> > +       int max_dp_lanes;
> >
> > -       /*
> > -        * Run with the maximum number of lanes that the DP sink supports.
> > -        *
> > -        * Depending use cases, we might want to revisit this later because:
> > -        * - It's plausible that someone may have run fewer lines to the
> > -        *   sink than the sink actually supports, assuming that the lines
> > -        *   will just be driven at a higher rate.
> > -        * - The DP spec seems to indicate that it's more important to minimize
> > -        *   the number of lanes than the link rate.
> > -        *
> > -        * If we do revisit, it would be important to measure the power impact.
> > -        */
> > -       pdata->dp_lanes = ti_sn_get_max_lanes(pdata);
> > +       max_dp_lanes = ti_sn_get_max_lanes(pdata);
> > +       pdata->dp_lanes = min(pdata->dp_lanes, max_dp_lanes);
> >
> >         /* DSI_A lane config */
> >         val = CHA_DSI_LANES(4 - pdata->dsi->lanes);
>
> Not a problem in this patch, but maybe this can be SN_MAX_DP_LANES -
> pdata->dsi->lanes now.

Since I introduce the define in this patch, I'll update it in v2.


> >         regmap_update_bits(pdata->regmap, SN_DSI_LANES_REG,
> >                            CHA_DSI_LANES_MASK, val);
> >
> > +       regmap_write(pdata->regmap, SN_LN_ASSIGN_REG, pdata->ln_assign);
> > +       regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, LN_POLRS_MASK,
> > +                          pdata->ln_polrs << LN_POLRS_OFFSET);
> > +
> >         /* set dsi clk frequency value */
> >         ti_sn_bridge_set_dsi_rate(pdata);
> >
> > @@ -1063,6 +1066,50 @@ static int ti_sn_setup_gpio_controller(struct ti_sn_bridge *pdata)
> >         return ret;
> >  }
> >
> > +static void ti_sn_bridge_parse_lanes(struct ti_sn_bridge *pdata,
> > +                                    struct device_node *np)
> > +{
> > +       u32 lane_assignments[SN_MAX_DP_LANES] = { 0, 1, 2, 3 };
> > +       u32 lane_polarities[SN_MAX_DP_LANES] = { };
> > +       struct device_node *endpoint;
> > +       u8 ln_assign = 0;
> > +       u8 ln_polrs = 0;
>
> Do we need to assign to 0 to start? Seems like no?

Yes.  See usage:

  ln_assign = ln_assign << LN_ASSIGN_WIDTH | lane_assignments[i];
  ln_polrs = ln_polrs << 1 | lane_polarities[i];

Notably each time we shift a new bit in we base on the old value.  If
you think it'll make it clearer, I can put this initialization at the
beginning of the loop.  It's 2 extra lines of code but if it adds
clarity I'll do it.


> > +       int dp_lanes;
> > +       int i;
> > +
> > +       /*
> > +        * Read config from the device tree about lane remapping and lane
> > +        * polarities.  These are optional and we assume identity map and
> > +        * normal polarity if nothing is specified.  It's OK to specify just
> > +        * data-lanes but not lane-polarities but not vice versa.
> > +        */
> > +       endpoint = of_graph_get_endpoint_by_regs(np, 1, -1);
> > +       dp_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
> > +       if (dp_lanes > 0) {
> > +               of_property_read_u32_array(endpoint, "data-lanes",
> > +                                          lane_assignments, dp_lanes);
> > +               of_property_read_u32_array(endpoint, "lane-polarities",
> > +                                          lane_polarities, dp_lanes);
> > +       } else {
> > +               dp_lanes = SN_MAX_DP_LANES;
> > +       }
>
> Needs an of_node_put(endpoint) here for the
> of_graph_get_endpoint_by_regs() above.

Thanks!  I'll fix in v2, which I'll send out either after a delay of a
few days or whenever I get resolution on my email to Laurent,
whichever comes first.  ;-)


-Doug