Re: [PATCH v2 3/5] phy: qcom-qmp: Add USB4 5NM QMP combo PHY registers

[Dmitry Baryshkov]

On Wed, 8 Jun 2022 at 02:02, Bjorn Andersson <bjorn.andersson@xxxxxxxxxx> wrote:
>
> On Tue 07 Jun 15:24 PDT 2022, Dmitry Baryshkov wrote:
>
> > On 08/06/2022 00:35, Bjorn Andersson wrote:
> > > Add all registers defines from qcom,usb4-5nm-qmp-combo.h of the msm-5.4
> > > kernel. Offsets are adjusted to be relative to each sub-block, as we
> > > describe the individual pieces in the upstream kernel and "v5_5NM" are
> > > injected in the defines to not collide with existing constants.
> > >
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> > > ---
> > >
> > > Changes since v1:
> > > - New patch
> > >
> > >   .../qualcomm/phy-qcom-usb4-5nm-qmp-combo.h    | 1547 +++++++++++++++++
> > >   1 file changed, 1547 insertions(+)
> > >   create mode 100644 drivers/phy/qualcomm/phy-qcom-usb4-5nm-qmp-combo.h
> > >
> > > diff --git a/drivers/phy/qualcomm/phy-qcom-usb4-5nm-qmp-combo.h b/drivers/phy/qualcomm/phy-qcom-usb4-5nm-qmp-combo.h
> > > new file mode 100644
> > > index 000000000000..7be8a50269ec
> > > --- /dev/null
> > > +++ b/drivers/phy/qualcomm/phy-qcom-usb4-5nm-qmp-combo.h
> > > @@ -0,0 +1,1547 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > +/*
> > > + * Copyright (c) 2021, The Linux Foundation. All rights reserved.
> > > + */
> > > +
> > > +#ifndef PHY_QCOM_V5_5NM_QMP_COMBO_USB4_H
> > > +#define PHY_QCOM_V5_5NM_QMP_COMBO_USB4_H
> > > +
> > > +/* USB4-USB3-DP Combo PHY register offsets */
> > > +/* Module: USB43DP_COM_USB43DP_COM_USB4_USB3_DP_COM */
> > > +#define USB43DP_V5_5NM_COM_PHY_MODE_CTRL                           0x00
> > > +#define USB43DP_V5_5NM_COM_SW_RESET                                        0x04
> > > +#define USB43DP_V5_5NM_COM_POWER_DOWN_CTRL                         0x08
> > > +#define USB43DP_V5_5NM_COM_SWI_CTRL                                        0x0c
> > > +#define USB43DP_V5_5NM_COM_TYPEC_CTRL                                      0x10
> > > +#define USB43DP_V5_5NM_COM_TYPEC_PWRDN_CTRL                                0x14
> > > +#define USB43DP_V5_5NM_COM_DP_BIST_CFG_0                           0x18
> > > +#define USB43DP_V5_5NM_COM_RESET_OVRD_CTRL1                                0x1c
> > > +#define USB43DP_V5_5NM_COM_RESET_OVRD_CTRL2                                0x20
> > > +#define USB43DP_V5_5NM_COM_DBG_CLK_MUX_CTRL                                0x24
> > > +#define USB43DP_V5_5NM_COM_TYPEC_STATUS                                    0x28
> > > +#define USB43DP_V5_5NM_COM_PLACEHOLDER_STATUS                              0x2c
> > > +#define USB43DP_V5_5NM_COM_REVISION_ID0                                    0x30
> > > +#define USB43DP_V5_5NM_COM_REVISION_ID1                                    0x34
> > > +#define USB43DP_V5_5NM_COM_REVISION_ID2                                    0x38
> > > +#define USB43DP_V5_5NM_COM_REVISION_ID3                                    0x3c
> >
> > QPHY_V5_DP_COM_foo ?
> >
>
> My first version of the QMP patch used V5 defines and USB worked
> sometimes. So I hacked up a thing to dump the phy sequences of the
> downstream and upstream kernels, compared the magic numbers and then
> tried to fit suitable constants.
>
> But it obviously was a waste of time and I would have to make up a
> different naming scheme for the ones that doesn't match the existing
> constants - when we could just use the autogenerated files that exist in
> the downstream kernels.

I decided that I should write more about it. My main issue with using
downstream tables is that we end up with tons of repetitive defines.
Each chip generation would bring 2-4 sets of tables, wouldn't it? This
can easily become an unsupported beast.
I'd propose to follow the opposite path. Let's split the existing
tables on a per-generation, per-region basis. Yes, we'd end up with
tens of the header files. However then when new generation arrives, we
can split corresponding header files on a region-by-region basis, and
compare each region with existing tables. If the region matches, use
it. If it does not, create a new header. Yes, I can do this for the
existing header as a continuation of the QMP split saga, if everybody
agrees that this is a good path.

You can ask, why do I suggest such a scheme? Because it looks like the
lowest common scheme. If we check downstream, we have USB/USB+DP with
huge autogenerated tables. Then comes UFS, which mostly follows naming
of the phy-qcom-qmp.h.

And the last one is a PCIe. I do not know about the sc8280xp, but for
the rest of the platforms we do not have register names at all. When I
was porting the SM8450 PCIe PHY support, I had to guess the correct
generation beforehand. With just 5 QSERDES_COM_ namespaces, guessing
is easy. If  we had separate namespaces for the UFS and for several
USB PHY instances, guessing would be next to impossible. And then
creating a correct table would also be impossible. Well, as long as we
do not accept tables without register names.

Thus I think we should resort to using a single naming scheme rather
than following downstream here. If you dislike existing
QSERDES_Vn/QPHY_Vn, let's come up with something more sensible.

--
With best wishes
Dmitry