Re: [PATCH 02/14] dt-bindings: phy: qcom,qmp-usb3-dp: fix sc8280xp bindings

[Dmitry Baryshkov]

On 14/11/2022 16:37, Johan Hovold wrote:
> On Sat, Nov 12, 2022 at 02:43:03PM +0300, Dmitry Baryshkov wrote:
> > On 11/11/2022 12:24, Johan Hovold wrote:
> > > The current QMP USB3-DP PHY bindings are based on the original MSM8996
> > > binding which provided multiple PHYs per IP block and these in turn were
> > > described by child nodes.
> > >
> > > The QMP USB3-DP PHY block provides a single multi-protocol PHY and even
> > > if some resources are only used by either the USB or DP part of the
> > > device there is no real benefit in describing these resources in child
> > > nodes.
> > >
> > > The original MSM8996 binding also ended up describing the individual
> > > register blocks as belonging to either the wrapper node or the PHY child
> > > nodes.
> > >
> > > This is an unnecessary level of detail which has lead to problems when
> > > later IP blocks using different register layouts have been forced to fit
> > > the original mould rather than updating the binding. The bindings are
> > > arguable also incomplete as they only the describe register blocks used
> > > by the current Linux drivers (e.g. does not include the PCS LANE
> > > registers).
> > >
> > > This is specifically true for later USB4-USB3-DP QMP PHYs where the TX
> > > registers are used by both the USB3 and DP parts of the PHY (and where
> > > the USB4 part of the PHY was not covered by the binding at all). Notably
> > > there are also no DP "RX" (sic) registers as described by the current
> > > bindings and the DP "PCS" region is really a set of DP_PHY registers.
> > >
> > > Add a new binding for the USB4-USB3-DP QMP PHYs found on SC8280XP which
> > > further bindings can be based on.
> > >
> > > Note that the binding uses a PHY type index to access either the USB3 or
> > > DP part of the PHY and that this can later be used also for the USB4
> > > part if needed.
> > >
> > > Similarly, the clock inputs and outputs can later be extended to support
> > > USB4.
> > >
> > > Also note that the current binding is simply removed instead of being
> > > deprecated as it was only recently merged and would not allow for
> > > supporting DP mode.
> > >
> > > Signed-off-by: Johan Hovold <johan+linaro@xxxxxxxxxx>
> > > ---
>
> > > +  "#clock-cells":
> > > +    const: 1
> > > +
> > > +  clock-output-names:
> > > +    items:
> > > +      - const: usb3_pipe
> > > +      - const: dp_link
> > > +      - const: dp_vco_div
> > > +
> > > +  "#phy-cells":
> > > +    const: 1
> > > +    description: |
> > > +      PHY index
> > > +        - PHY_TYPE_USB3
> > > +        - PHY_TYPE_DP
> >
> > I'm stepping on Rob's and Krzysztof's ground here, but it might be more
> > logical and future proof to use indices instead of phy types.
>
> Why would that be more future-proof?
>
> I initially added defines for these indexes to a QMP header, but noticed
> that we already have PHY drivers that use the PHY types for this. So
> there's already a precedent for this and I didn't see any real benefit
> to adding multiple per-SoC defines for the same thing.

As you guessed from my question, I was thinking about USB4 (for which we 
do not have a separate PHY_TYPE, but that probably shouldn't matter). 
Would it be a separate PHY here, or would it be a combo UBS3+USB4 plus 
separate DP phy?

Yes, we have other PHYs, which use types as an index, however it's 
slightly more common to have indices instead.

Anyway, this is a minor issue. It might be just that I'm more common to 
using indices everywhere (in other words, I have preference here, but 
it's not a strong requirement from my side).


> > Just for my understanding, would USB4 support add another qserdes+tx/rx
> > construct or would it be the same USB3 register space?
>
> The TX/RX registers are shared by the all three parts of the PHY (USB4,
> USB3, DP), while USB4 has two dedicated sets of PLL (serdes) and PCS
> registers.

Ack, thanks.

--
With best wishes
Dmitry