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
