Chandana Kishori Chiluveru has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/25213 )
Change subject: sdm845: Add USB support on cheza platform ......................................................................
Patch Set 65:
(7 comments)
https://review.coreboot.org/#/c/25213/64/src/mainboard/google/cheza/mainboar... File src/mainboard/google/cheza/mainboard.c:
https://review.coreboot.org/#/c/25213/64/src/mainboard/google/cheza/mainboar... PS64, Line 30: /* Setting up Secondary USB DWC3 controller. */
nit: maybe mention that the primary isn't needed here because it's only used for DP on Cheza?
Ok
i will modify this as below
/* * Primary USB is used only for DP functionality on cheza platform. * Hence Setting up only Secondary USB DWC3 controller. */
https://review.coreboot.org/#/c/25213/64/src/mainboard/google/cheza/romstage... File src/mainboard/google/cheza/romstage.c:
https://review.coreboot.org/#/c/25213/64/src/mainboard/google/cheza/romstage... PS64, Line 22: these resets : * off early so they get
nit: singular now
"these resets" here i mean is core+ PHYs resets which is plural here. reset_usb() on usb.c driver takes care of all the resets(usb core+qub phy+qmp phy) required for usb.
/* Assert Core reset */ clock_reset_bcr(dwc3->usb3_bcr, 1);
/* Assert QUSB PHY reset */ clock_reset_bcr(dwc3->qusb2phy_bcr, 1);
/* Assert QMP PHY reset */ clock_reset_bcr(dwc3->gcc_usb3phy_bcr_reg, 1); clock_reset_bcr(dwc3->gcc_qmpphy_bcr_reg, 1);
https://review.coreboot.org/#/c/25213/64/src/soc/qualcomm/sdm845/usb.c File src/soc/qualcomm/sdm845/usb.c:
https://review.coreboot.org/#/c/25213/64/src/soc/qualcomm/sdm845/usb.c@359 PS64, Line 359: static const struct qmp_phy_init_tbl qmp_v3_usb3_serdes_tbl[] = {
Okay, this is too much duplication. […]
for phy initialization order should be important. we can not change the sequence in SW which is suggested in HPG.
if you remember the ss change which i pushed separatly i used the same logic which you suggested here
https://review.coreboot.org/c/coreboot/+/29200/6/src/soc/qualcomm/sdm845/usb...
static void qcom_qmp_phy_configure(void *base, const struct qmp_phy_init_tbl tbl[], int num) { int i; const struct qmp_phy_init_tbl *t = tbl;
if (!t) return;
for (i = 0; i < num; i++, t++) write32(base + t->offset, t->val); }
static void ss_qmp_uni_phy_init(struct usb_dwc3_cfg *dwc3) { struct stopwatch sw; void *phy_base = dwc3->qmp_phy_base;
/* power up USB3 PHY */ write32(phy_base + USB3_UNI_POWER_DOWN_CONTROL, 0x01);
/* Serdes configuration */ qcom_qmp_phy_configure(&dwc3->qmp_phy_base->usb3_uniphy_qmp_pll_reg, dwc3->serdes_tbl, dwc3->serdes_tbl_num); /* Tx, Rx, and PCS configurations */ qcom_qmp_phy_configure(&dwc3->qmp_phy_base->usb3_uniphy_qmp_tx, dwc3->tx_tbl, dwc3->tx_tbl_num);
qcom_qmp_phy_configure(&dwc3->qmp_phy_base->usb3_uniphy_qmp_rx, dwc3->rx_tbl, dwc3->rx_tbl_num);
qcom_qmp_phy_configure(&dwc3->qmp_phy_base->usb3_uniphy_qmp_pcs, dwc3->pcs_tbl, dwc3->pcs_tbl_num);
/* perform software reset of PCS/Serdes */ write32(phy_base + USB3_UNI_SW_RESET, 0x00); /* start PCS/Serdes to operation mode */ write32(phy_base + USB3_UNI_START, 0x03);
I did these changes again to make use of registers from the structures.
https://review.coreboot.org/#/c/25213/64/src/soc/qualcomm/sdm845/usb.c@467 PS64, Line 467: usb3_uniphy
nit: it's a little confusing that these structs are all called "usb3" and "usb3_uniphy"... […]
Here usb_uniphy is uniphy which is used for secondary port. USB3 phy is primay phy( USB+DP phy).
we can use this macro directly to search the registers related to the phy qmp in HPG document for SW programming.
https://review.coreboot.org/#/c/25213/64/src/soc/qualcomm/sdm845/usb.c@585 PS64, Line 585: int serdes_tbl_num;
Only one table (the first one with the differences) needs to remain here if you combine them as ment […]
we can not do this as we have differences in all the regsiter sets(serdes, tx,rx,pcs).
below are the differences for q phys
{&qserdes_com_reg_layout->com_bias_en_clkbuflr_en, 0x08}, {&qserdes_com_reg_layout->com_cmn_config, 0x16},
{&uniphy_qserdes_com_reg_layout->com_bias_en_clkbuflr_en, 0x04}, {&uniphy_qserdes_com_reg_layout->com_cmn_config, 0x06},
{&qserdes_tx_reg_layout->tx_lane_mode_1, 0x16}, {&qserdes_tx_reg_layout->tx_res_code_lane_offset_rx, 0x09},
{&uniphy_qserdes_tx_reg_layout->tx_lane_mode_1, 0xc6}, {&uniphy_qserdes_tx_reg_layout->tx_res_code_lane_offset_rx, 0x06},
{&qserdes_rx_reg_layout->rx_rx_equ_adap_ctrl2, 0x0f}, {&qserdes_rx_reg_layout->rx_sigdet_deglitch_ctrl, 0x16},
{&uniphy_qserdes_rx_reg_layout->rx_vga_cal_ctrl2, 0x0c}, {&uniphy_qserdes_rx_reg_layout->rx_rx_mode_00, 0x50}, {&uniphy_qserdes_rx_reg_layout->rx_rx_equ_adap_ctrl2, 0x0e}, {&uniphy_qserdes_rx_reg_layout->rx_sigdet_deglitch_ctrl, 0x1c},
{&pcs_reg_layout->pcs_txmgn_v2, 0xb7}, {&pcs_reg_layout->pcs_txmgn_v3, 0x4e}, {&pcs_reg_layout->pcs_txmgn_v4, 0x65}, {&pcs_reg_layout->pcs_txmgn_ls, 0x6b},
{&uniphy_pcs_reg_layout->pcs_txmgn_v2, 0xb5}, {&uniphy_pcs_reg_layout->pcs_txmgn_v3, 0x4c}, {&uniphy_pcs_reg_layout->pcs_txmgn_v4, 0x64}, {&uniphy_pcs_reg_layout->pcs_txmgn_ls, 0x6a} {&uniphy_pcs_reg_layout->pcs_refgen_req_config1, 0x21}, {&uniphy_pcs_reg_layout->pcs_refgen_req_config2, 0x60},
https://review.coreboot.org/#/c/25213/64/src/soc/qualcomm/sdm845/usb.c@782 PS64, Line 782: !
Wait, just to double-check: are we waiting for the bit to become 1 or 0? wait_us() waits for the con […]
I have checked this and as you said we need to check vstatus bit to become 1. i will correct it.
HPG DOC: QUSB2PHY_DEBUG_STAT5 wait for 88 uS for PLL lock; vstatus[0] changes from 0 to 1
I will the comment like below for this
/* DEBUG_STAT5: wait for 160uS for PLL lock; vstatus[0] changes from 0 to 1 */ long lock_us = wait_us(160, (read32(&dwc3->qusb_phy_dig->debug_stat5) & VSTATUS_PLL_LOCK_STATUS_MASK)); if (!lock_us) printk(BIOS_ERR, "ERROR: QUSB PHY PLL LOCK fails\n"); else printk(BIOS_DEBUG, "QUSB PHY initialized and locked in %ldus\n", lock_us);
https://review.coreboot.org/#/c/25213/64/src/soc/qualcomm/sdm845/usb.c@824 PS64, Line 824: USB3_PCS_PHYSTATUS));
Again, this waits for PHYSTATUS to become 0, please make sure that's actually what you mean.
Yes, SW wait untill phystatus become 0.
you can refer the Software programming guide shared for phy init sequences.
Read PCS_PCS_STATUS This can be done either of the two ways: 1) Finite delay can be added before observing PHYSTATUS = 1’b0 2) SW can continuously poll for PHYSTATUS = 1’b0
I will add this info as comment below
/* Wait for PHY initialization to be done * PCS_STATUS: wait for 1ms for PHY STATUS; * SW can continuously check for PHYSTATUS = 1.b0 */ long lock_us = wait_us(1000, !(read32(&dwc3->qmp_pcs_reg->pcs_ready_status) & USB3_PCS_PHYSTATUS));