Re: [PATCH v2 2/5] phy: qcom-qmp: Add USB3 5NM QMP UNI registers

[Dmitry Baryshkov]

On Wed, 8 Jun 2022 at 01:43, Bjorn Andersson <bjorn.andersson@xxxxxxxxxx> wrote:
>
> On Tue 07 Jun 14:58 PDT 2022, Dmitry Baryshkov wrote:
> > On 08/06/2022 00:35, Bjorn Andersson wrote:
> [..]
> > > +#define USB3_V5_5NM_UNI_QSERDES_COM_BIN_VCOCAL_CMP_CODE2_MODE0     0x1b0
> > > +#define USB3_V5_5NM_UNI_QSERDES_COM_BIN_VCOCAL_CMP_CODE1_MODE1     0x1b4
> > > +#define USB3_V5_5NM_UNI_QSERDES_COM_BIN_VCOCAL_CMP_CODE2_MODE1     0x1b8
> > > +#define USB3_V5_5NM_UNI_QSERDES_COM_BIN_VCOCAL_HSCLK_SEL           0x1bc
> > > +#define USB3_V5_5NM_UNI_QSERDES_COM_RESERVED_1                     0x1c0
> > > +#define USB3_V5_5NM_UNI_QSERDES_COM_MODE_OPERATION_STATUS          0x1c4
> >
> > These defines look completely compatible with the existing ones in the
> > QSERDES_V5_COM_ namespace. Please use them instead.
> >
>
> Can you please confirm that all these constants are exactly the same as
> the existing V5 entries?

The only difference that I see is the phy-qcom-qmp.h defining
QSERDES_V5_COM_CMN_MODE to 0x1a4, which should be 0x1a0. This is
clearly a mistake on the upstream side (confirmed by the msm-5.10).
Could you please send a patch for it?


>
> [..]
> > > +/* Module: USB3_UNI_PCS_USB3_PCIE_USB3_UNI_PCS_USB3 */
> > > +#define USB3_V5_5NM_UNI_PCS_USB3_POWER_STATE_CONFIG1               0x00
> > > +#define USB3_V5_5NM_UNI_PCS_USB3_AUTONOMOUS_MODE_STATUS            0x04
> > > +#define USB3_V5_5NM_UNI_PCS_USB3_AUTONOMOUS_MODE_CTRL              0x08
> > > +#define USB3_V5_5NM_UNI_PCS_USB3_AUTONOMOUS_MODE_CTRL2             0x0c
> > > +#define USB3_V5_5NM_UNI_PCS_USB3_LFPS_RXTERM_IRQ_SOURCE_STATUS     0x10
> > > +#define USB3_V5_5NM_UNI_PCS_USB3_LFPS_RXTERM_IRQ_CLEAR             0x14
> > > +#define USB3_V5_5NM_UNI_PCS_USB3_LFPS_DET_HIGH_COUNT_VAL           0x18
> > > +#define USB3_V5_5NM_UNI_PCS_USB3_LFPS_TX_ECSTART                   0x1c
> > > +#define USB3_V5_5NM_UNI_PCS_USB3_LFPS_PER_TIMER_VAL                0x20
> > > +#define USB3_V5_5NM_UNI_PCS_USB3_LFPS_TX_END_CNT_U3_START          0x24
> > > +#define USB3_V5_5NM_UNI_PCS_USB3_LFPS_CONFIG1                      0x28
> > > +#define USB3_V5_5NM_UNI_PCS_USB3_RXEQTRAINING_LOCK_TIME            0x2c
> > > +#define USB3_V5_5NM_UNI_PCS_USB3_RXEQTRAINING_WAIT_TIME            0x30
> > > +#define USB3_V5_5NM_UNI_PCS_USB3_RXEQTRAINING_CTLE_TIME            0x34
> > > +#define USB3_V5_5NM_UNI_PCS_USB3_RXEQTRAINING_WAIT_TIME_S2         0x38
> > > +#define USB3_V5_5NM_UNI_PCS_USB3_RXEQTRAINING_DFE_TIME_S2          0x3c
> > > +#define USB3_V5_5NM_UNI_PCS_USB3_RCVR_DTCT_DLY_U3_L                0x40
> > > +#define USB3_V5_5NM_UNI_PCS_USB3_RCVR_DTCT_DLY_U3_H                0x44
> > > +#define USB3_V5_5NM_UNI_PCS_USB3_ARCVR_DTCT_EN_PERIOD              0x48
> > > +#define USB3_V5_5NM_UNI_PCS_USB3_ARCVR_DTCT_CM_DLY                 0x4c
> > > +#define USB3_V5_5NM_UNI_PCS_USB3_TXONESZEROS_RUN_LENGTH            0x50
> > > +#define USB3_V5_5NM_UNI_PCS_USB3_ALFPS_DEGLITCH_VAL                0x54
> > > +#define USB3_V5_5NM_UNI_PCS_USB3_SIGDET_STARTUP_TIMER_VAL          0x58
> > > +#define USB3_V5_5NM_UNI_PCS_USB3_TEST_CONTROL                      0x5c
> > > +#define USB3_V5_5NM_UNI_PCS_USB3_RXTERMINATION_DLY_SEL             0x60
> >
> > These look like QPHY_V5_PCS_USB3, but without additional 0x300 offset. I'd
> > suggest modifying qcom-qmp-phy-usb.c to allocate another register space for
> > pcs_usb and updating QPHY_V4_PCS_USB3_foo / QPHY_V5_PCS_USB3_foo defines to
> > remove this offset.
> >
> > Afterwards most if not all constants from this header can be merged into
> > phy-qcom-qmp.h I do not think that it makes sense to split this header at
> > this moment. The QSERDES_COM/_TX/_RX/_PCS defines are common to all PHY
> > types.
> >
>
> You might be right, but I spent considerable time debugging the combo
> phy (which is version 5.0.0) and in the end it turned out that it's not
> the same offsets.
>
> I really would prefer that we stop haphazardly try to fit things into
> the phy-qcom-qmp.h with version numbers that we essentially make up
> base, when Qualcomm dumps the register layout for each generation in
> their downstream kernel.

Well... Let's come up with a better versioning/naming scheme. But I
don't think we should dump repeatable symbol headers, which are in
reality common between different PHYs. This would make things harder
to understand and harder to maintain.

-- 
With best wishes
Dmitry