Re: [PATCH v1 2/2] phy: phy-cadence-torrent: Use PHY kernel APIs to set PHY attributes

[Maxime Ripard]

Hi!

On Thu, Apr 30, 2020 at 02:06:20PM +0000, Swapnil Kashinath Jakhade wrote:
> Thank you so much for reviewing the patch. Please see inline reply below.
> 
> > -----Original Message-----
> > From: Maxime Ripard <maxime@xxxxxxxxxx>
> > Sent: Wednesday, April 29, 2020 5:58 PM
> > To: Yuti Suresh Amonkar <yamonkar@xxxxxxxxxxx>
> > Cc: linux-kernel@xxxxxxxxxxxxxxx; kishon@xxxxxx; mark.rutland@xxxxxxx;
> > jsarha@xxxxxx; tomi.valkeinen@xxxxxx; praneeth@xxxxxx; Milind Parab
> > <mparab@xxxxxxxxxxx>; Swapnil Kashinath Jakhade
> > <sjakhade@xxxxxxxxxxx>
> > Subject: Re: [PATCH v1 2/2] phy: phy-cadence-torrent: Use PHY kernel APIs to
> > set PHY attributes
> > 
> > EXTERNAL MAIL
> > 
> > 
> > Hi,
> > 
> > On Tue, Apr 28, 2020 at 09:10:04AM +0200, Yuti Amonkar wrote:
> > > From: Swapnil Jakhade <sjakhade@xxxxxxxxxxx>
> > >
> > > Use generic PHY framework function phy_set_bus_width() to set number
> > > of lanes and function phy_set_max_link_rate() to set maximum link rate
> > > supported by PHY.
> > >
> > > Signed-off-by: Swapnil Jakhade <sjakhade@xxxxxxxxxxx>
> > 
> > This should have your SoB.
> > 
> > > ---
> > >  drivers/phy/cadence/phy-cadence-torrent.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/phy/cadence/phy-cadence-torrent.c
> > b/drivers/phy/cadence/phy-cadence-torrent.c
> > > index 7116127358ee..b914e5ddf93c 100644
> > > --- a/drivers/phy/cadence/phy-cadence-torrent.c
> > > +++ b/drivers/phy/cadence/phy-cadence-torrent.c
> > > @@ -1852,6 +1852,9 @@ static int cdns_torrent_phy_probe(struct
> > platform_device *pdev)
> > >  				 cdns_phy->phys[node].num_lanes,
> > >  				 cdns_phy->max_bit_rate / 1000,
> > >  				 cdns_phy->max_bit_rate % 1000);
> > > +
> > > +			phy_set_bus_width(gphy, cdns_phy-
> > >phys[node].num_lanes);
> > > +			phy_set_max_link_rate(gphy, cdns_phy-
> > >max_bit_rate);
> > 
> > I think what Kishon meant in his previous mail is that it's not really clear (to
> > me at least) how that function would be useful.
> > 
> > In this particular case, what would the consumer make of that information?
> > Does
> > the phy needs to be reconfigured based on the max rate being changed?
> > 
> > Some phy_configure_opts structures also have a somewhat similar field that
> > can
> > be negociated between the provider and the consumer using phy_validate,
> > wouldn't
> > that be redundant?
> > 
> > Most of that discussion can only happen when you've provided a use-case
> > for that
> > series, so a consumer using it would help greatly there.
> 
> Actually, for this particular case, consumer driver will be the Cadence MHDP
> bridge driver for DisplayPort which is also under review process for
> upstreaming [1]. So this DRM bridge driver will make use of the PHY APIs
> phy_get_bus_width() and phy_get_max_link_rate() during execution of probe
> function to get the number of lanes and maximum link rate supported by Cadence
> Torrent PHY. This information is required to set the host capabilities in the
> DRM bridge driver, based on which initial values for DisplayPort link training
> will be determined.
>
> The changes in this PHY patch series are based on suggestions in the review
> comments in [1] which asks to use kernel PHY APIs to read these properties
> instead of directly accessing PHY device node. The complete driver and actual
> use of these APIs can be found in [2]. This is how we are planning to use
> these APIs.

I haven't really looked into the displayport spec, but I'd assume that there's a
lot more parameters that would need to be negociated between the phy and the DP
block? If so, then it would make more sense to follow the path we did for
MIPI-DSI where the parameters can be negociated through the phy_configure /
phy_validate interface.

Maxime

Attachment: signature.asc
Description: PGP signature