Re: [PATCH v2] phy: Add DisplayPort configuration options

From: Maxime Ripard
Date: Thu Jan 02 2020 - 05:37:02 EST


Hi,

On Tue, Dec 24, 2019 at 12:29:40PM +0000, Yuti Suresh Amonkar wrote:
> > -----Original Message-----
> > From: Maxime Ripard <maxime@xxxxxxxxxx>
> > Sent: Monday, December 23, 2019 22:49
> > To: Yuti Suresh Amonkar <yamonkar@xxxxxxxxxxx>
> > Cc: linux-kernel@xxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
> > praneeth@xxxxxx; tomi.valkeinen@xxxxxx; jsarha@xxxxxx; Milind Parab
> > <mparab@xxxxxxxxxxx>; Swapnil Kashinath Jakhade
> > <sjakhade@xxxxxxxxxxx>
> > Subject: Re: [PATCH v2] phy: Add DisplayPort configuration options
> >
> > EXTERNAL MAIL
> >
> >
> > Hi,
> >
> > Please note that I don't have access to the displayPort spec, so I'll only
> > comment on the content of that patch, not whether it's complete or not.
> >
> > On Mon, Dec 23, 2019 at 02:41:13PM +0100, Yuti Amonkar wrote:
> > > Allow DisplayPort PHYs to be configured through the generic functions
> > > through a custom structure added to the generic union.
> > > The configuration structure is used for reconfiguration of DisplayPort
> > > PHYs during link training operation.
> > >
> > > The parameters added here are the ones defined in the DisplayPort spec
> > > 1.4 which include link rate, number of lanes, voltage swing and
> > > pre-emphasis.
> > >
> > > Signed-off-by: Yuti Amonkar <yamonkar@xxxxxxxxxxx>
> > > ---
> > >
> > > This patch was a part of [1] series earlier but we think that it needs
> > > to have a separate attention of the reviewers. Also as both [1] & [2]
> > > are dependent on this patch, our sincere request to reviewers to have
> > > a faster review of this patch.
> > >
> > > [1]
> > >
> > > https://lkml.org/lkml/2019/12/11/455
> > >
> > > [2]
> > >
> > > https://patchwork.kernel.org/cover/11271191/
> > >
> > > include/linux/phy/phy-dp.h | 95
> > ++++++++++++++++++++++++++++++++++++++++++++++
> > > include/linux/phy/phy.h | 4 ++
> > > 2 files changed, 99 insertions(+)
> > > create mode 100644 include/linux/phy/phy-dp.h
> > >
> > > diff --git a/include/linux/phy/phy-dp.h b/include/linux/phy/phy-dp.h
> > > new file mode 100644 index 0000000..18cad23
> > > --- /dev/null
> > > +++ b/include/linux/phy/phy-dp.h
> > > @@ -0,0 +1,95 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * Copyright (C) 2019 Cadence Design Systems Inc.
> > > + */
> > > +
> > > +#ifndef __PHY_DP_H_
> > > +#define __PHY_DP_H_
> > > +
> > > +#include <linux/types.h>
> > > +
> > > +/**
> > > + * struct phy_configure_opts_dp - DisplayPort PHY configuration set
> > > + *
> > > + * This structure is used to represent the configuration state of a
> > > + * DisplayPort phy.
> > > + */
> > > +struct phy_configure_opts_dp {
> > > + /**
> > > + * @link_rate:
> > > + *
> > > + * Link Rate, in Mb/s, of the main link.
> > > + *
> > > + * Allowed values: 1620, 2160, 2430, 2700, 3240, 4320, 5400, 8100
> > Mb/s
> > > + */
> > > + unsigned int link_rate;
> > > +
> > > + /**
> > > + * @lanes:
> > > + *
> > > + * Number of active, consecutive, data lanes, starting from
> > > + * lane 0, used for the transmissions on main link.
> > > + *
> > > + * Allowed values: 1, 2, 4
> > > + */
> > > + unsigned int lanes;
> > > +
> > > + /**
> > > + * @voltage:
> > > + *
> > > + * Voltage swing levels, as specified by DisplayPort specification,
> > > + * to be used by particular lanes. One value per lane.
> > > + * voltage[0] is for lane 0, voltage[1] is for lane 1, etc.
> > > + *
> > > + * Maximum value: 3
> > > + */
> > > + unsigned int voltage[4];
> > > +
> > > + /**
> > > + * @pre:
> > > + *
> > > + * Pre-emphasis levels, as specified by DisplayPort specification, to be
> > > + * used by particular lanes. One value per lane.
> > > + *
> > > + * Maximum value: 3
> > > + */
> > > + unsigned int pre[4];
> > > +
> > > + /**
> > > + * @ssc:
> > > + *
> > > + * Flag indicating, whether or not to enable spread-spectrum
> > clocking.
> > > + *
> > > + */
> > > + u8 ssc : 1;
> > > +
> > > + /**
> > > + * @set_rate:
> > > + *
> > > + * Flag indicating, whether or not reconfigure link rate and SSC to
> > > + * requested values.
> > > + *
> > > + */
> > > + u8 set_rate : 1;
> > > +
> > > + /**
> > > + * @set_lanes:
> > > + *
> > > + * Flag indicating, whether or not reconfigure lane count to
> > > + * requested value.
> > > + *
> > > + */
> > > + u8 set_lanes : 1;
> > > +
> > > + /**
> > > + * @set_voltages:
> > > + *
> > > + * Flag indicating, whether or not reconfigure voltage swing
> > > + * and pre-emphasis to requested values. Only lanes specified
> > > + * by "lanes" parameter will be affected.
> > > + *
> > > + */
> > > + u8 set_voltages : 1;
> >
> > I'm not quite sure what these flags are supposed to be doing, or what use-
> > cases they cover. The current API is using validate to make sure that we can
> > have a handshake between the caller and its PHY and must never apply the
> > configuration, and configure must always apply the configuration. These
> > flags look redundant.
> >
> > Maxime
>
> These flags are used to reconfigure the link during the link
> training procedure as described in DisplayPort spec. In this
> procedure , we may need to configure only subset of parameters (VS,
> Pre-emphasis, link rate and num of lanes) depending on different
> phases. e.g. At one stage, we may need to configure only Voltage
> swing and Pre-emphasis keeping number of lanes and link rate
> intact(set_voltages=true), while at other stage, we may need to
> configure all parameters. We use the flags to determine which
> parameter is updated during link training. Using separate flags for
> this provides control to upper layer.

Ok, it makes sense then :)

> I am not sure how to use validate to achieve this. As per my
> understanding validate is used to verify if set of parameters can be
> handled by PHY.

That's correct :)

Maxime

Attachment: signature.asc
Description: PGP signature