Re: [PATCH v4 2/4] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips

[Vivek Gautam]

Hi,


On 03/07/2017 02:34 PM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Thursday 02 March 2017 10:10 PM, Vivek Gautam wrote:
> > Hi Kishon,
> >
> >
> > On Wed, Feb 22, 2017 at 9:29 AM, Vivek Gautam
> > <vivek.gautam@xxxxxxxxxxxxxx> wrote:
> > > Hi Kishon,
> > >
> > >
> > > On Fri, Jan 27, 2017 at 11:54 AM, Vivek Gautam
> > > <vivek.gautam@xxxxxxxxxxxxxx> wrote:
> > > > On 01/26/2017 11:45 PM, Bjorn Andersson wrote:
> > > > > On Tue 24 Jan 01:19 PST 2017, Kishon Vijay Abraham I wrote:
> > > > > > On Monday 23 January 2017 03:43 PM, Vivek Gautam wrote:
> > > > > > > On Wed, Jan 18, 2017 at 11:33 PM, Bjorn Andersson
> > > > > [..]
> > > > > > > Yes, that's correct. The QMP and QUSB2 phy init sequences are a bunch
> > > > > > > of static values for a particular IP version. These values hardly give a
> > > > > > > meaningful data to put few phy bindings that could be referenced
> > > > > > > to configure the phy further.
> > > > > > Not really. You can have clearly defined phy binding to give meaningful
> > > > > > data.
> > > > > > Every driver doing the same configuration bloats the driver and these
> > > > > > configuration values are just magic values which hardly can be reviewed
> > > > > > by anyone.
> > > > > > > > Further more moving this blob to devicetree will not allow us to treat
> > > > > > > > the various QMP configurations as one HW block, as there are other
> > > > > > > > differences as well.
> > > > > > > >
> > > > > > > > Like many other drivers it's possible to create a generic version that
> > > > > > > > has every bit of logic driven by configuration from devicetree, but
> > > > > > > > like
> > > > > > > > most of those cases this is not the way we split things.
> > > > > > > >
> > > > > > > > And this has the side effect of keeping the dts files human readable,
> > > > > > > > human understandable and human maintainable.
> > > > > > right. That's why I recommend having clearly defined bindings.
> > > > > >          phy,tx-<param1> = <val, offset, mask>
> > > > > >          phy,tx-<param2> = <val, offset, mask>
> > > > > >          phy,tx-<param3> = <val, offset, mask>
> > > > > There's no doubt that this table needs to be encoded somewhere, so the
> > > > > question is should we hard code this in a C file or in a DTSI file.
> > > > >
> > > > > Skimming through [1] I see examples of things that differs based on how
> > > > > the specific component is integrated in a SoC or on a particular board -
> > > > > properties that are relevant to a "system integrator".
> > > > >
> > > > > As far as I can tell this blob will, if ever, only be changed by a
> > > > > driver developer and as such it's not carry information about how this
> > > > > component relates to the rest of the system and should as such not be
> > > > > part of the device tree.
> > > > >
> > > > >
> > > > > If there are properties of the hardware that is affected by how the
> > > > > component is integrated in the system I really would like for those to
> > > > > be exposed as human-readable properties that I can understand and alter
> > > > > without deep knowledge about the register map of the hardware block.
> > > >
> > > > I am reaching out to our internal teams to get more information
> > > > on different possible phy configurations, based on which the registers
> > > > values are decided.
> > > > This is something that i tried to understand in the past as well, but
> > > > couldn't
> > > > grab much information that time.
> > > > Will come back with relevant information on this.
> > > We have started looking into understanding the PHYs on msm and
> > > eventually create a set of generic phy bindings that can serve multiple
> > > platforms.
> > > But this task, I presume, will take its course and will involve multi-party
> > > discussions.
> > >
> > > For these QUSB2 and QMP phy drivers, a good amount of work has
> > > already gone in getting these drivers in upstream state.
> > > The common QMP phy driver supports a bunch of controllers on msm
> > > platforms - USB, PCIe and UFS and there are platforms such as DB820c
> > > and others that want to pull in these changes from upstream.
> > > The future phy controllers also depend on these drivers and we don't want
> > > to hold other developers to contribute to these drivers.
> > > So, we wish to not delay these drivers further because of the phy bindings.
> > > I see that there are phys that still program the registers-value pairs for
> > > phy initialization.
> > >
> > > We will keep working on the bindings while these patches
> > > make way to upstream.
> > >
> > > Can you consider pulling in these drivers?
> > > I can send the next version of these drivers with other comments addressed.
> > > Please let me know your comments.
> > Gentle ping. Any thoughts on this ?
> sure, lets hold the phy configuration binding and complete the rest of the patch.

Sure, thanks. Will post out the next version of patches soon.

Regards
Vivek

> Thanks
> Kishon

--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project