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
