mbox series

[00/15] ufs: qcom: Add HS-G4 support

Message ID 20221029141633.295650-1-manivannan.sadhasivam@linaro.org
Headers show
Series ufs: qcom: Add HS-G4 support | expand

Message

Manivannan Sadhasivam Oct. 29, 2022, 2:16 p.m. UTC
Hello,

This series adds HS-G4 support to the Qcom UFS driver and PHY driver.
The newer Qcom platforms support configuring the UFS controller and PHY
in dual gears (i.e., controller/PHY can be configured to run in two gear
speeds). This is accomplished by adding two different PHY init sequences
to the PHY driver and the UFS driver requesting the one that's required
based on the platform configuration.

But this requires both the UFS controller and UFS device to agree to a
common gear. For finding the max supported gear, a separate register is
used for the UFS controller and devicetree is used for the UFS device.
Based on the max gear of both, the UFS driver will decide which gear to
use during runtime.

This series has been tested on Qcom RB5 development platform powered by
SM8250 SoC that uses HS-G4.

Merging Strategy:
-----------------

The PHY patches are expected to go through PHY tree and UFS, MAINTAINERS
patches are expected to go through SCSI tree. Finally, the binding and
devicetree patches can go through ARM MSM tree. There is no build dependency
between the patches.

Thanks,
Mani

Manivannan Sadhasivam (15):
  phy: qcom-qmp-ufs: Move register settings to qmp_phy_cfg_tables struct
  phy: qcom-qmp-ufs: Add support for configuring PHY in HS Series B mode
  phy: qcom-qmp-ufs: Add support for configuring PHY in HS G4 mode
  phy: qcom-qmp-ufs: Add HS G4 mode support to SM8250 SoC
  phy: qcom-qmp-ufs: Move HS Rate B register setting to tables_hs_b
  dt-bindings: ufs: Add "max-gear" property for UFS device
  arm64: dts: qcom: qrb5165-rb5: Add max-gear property to UFS node
  scsi: ufs: ufs-qcom: Remove un-necessary goto statements
  scsi: ufs: ufs-qcom: Remove un-necessary WARN_ON()
  scsi: ufs: ufs-qcom: Use bitfields where appropriate
  scsi: ufs: ufs-qcom: Use dev_err_probe() for printing probe error
  scsi: ufs: ufs-qcom: Fix the Qcom register name for offset 0xD0
  scsi: ufs: ufs-qcom: Factor out the logic finding the HS Gear
  scsi: ufs: ufs-qcom: Add support for finding HS gear on new UFS
    versions
  MAINTAINERS: Add myself as the maintainer for Qcom UFS driver

 .../devicetree/bindings/ufs/ufs-common.yaml   |   5 +
 MAINTAINERS                                   |   8 +
 arch/arm64/boot/dts/qcom/qrb5165-rb5.dts      |   1 +
 drivers/phy/qualcomm/phy-qcom-qmp-ufs.c       | 350 +++++++++++++-----
 drivers/ufs/host/ufs-qcom.c                   | 177 +++++----
 drivers/ufs/host/ufs-qcom.h                   |  64 ++--
 6 files changed, 389 insertions(+), 216 deletions(-)

Comments

Dmitry Baryshkov Oct. 29, 2022, 9:48 p.m. UTC | #1
On 29/10/2022 17:16, Manivannan Sadhasivam wrote:
> Starting from UFS controller v4, Qcom supports dual gear mode (i.e., the
> controller/PHY can be configured to run in two gear speeds). But that
> requires an agreement between the UFS controller and the UFS device.
> This commit finds the max gear supported by both controller and device
> then decides which one to use.
> 
> UFS controller's max gear can be read from the REG_UFS_PARAM0 register and
> UFS device's max gear can be read from the "max-gear" devicetree property.
> 
> The UFS PHY also needs to be configured with the decided gear using the
> phy_set_mode_ext() API.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   drivers/ufs/host/ufs-qcom.c | 35 ++++++++++++++++++++++++++++++++---
>   drivers/ufs/host/ufs-qcom.h |  4 ++++
>   2 files changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index f952cc76919f..268463e92d67 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -281,6 +281,9 @@ static int ufs_qcom_host_reset(struct ufs_hba *hba)
>   static u32 ufs_qcom_get_hs_gear(struct ufs_hba *hba, u32 hs_gear)
>   {
>   	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> +	struct device *dev = hba->dev;
> +	u32 max_gear, hcd_max_gear, reg;
> +	int ret;
>   
>   	if (host->hw_ver.major == 0x1) {
>   		/*
> @@ -292,8 +295,33 @@ static u32 ufs_qcom_get_hs_gear(struct ufs_hba *hba, u32 hs_gear)
>   		 */
>   		if (hs_gear > UFS_HS_G2)
>   			return UFS_HS_G2;
> +	} else if (host->hw_ver.major > 0x3) {
> +		/*
> +		 * Starting from UFS controller v4, Qcom supports dual gear mode (i.e., the
> +		 * controller/PHY can be configured to run in two gear speeds). But that
> +		 * requires an agreement between the UFS controller and the device. Below
> +		 * code tries to find the max gear of both and decides which gear to use.
> +		 *
> +		 * First get the max gear supported by the UFS device if available.
> +		 * If the property is not defined in devicetree, then use the default gear.
> +		 */
> +		ret = of_property_read_u32(dev->of_node, "max-gear", &max_gear);
> +		if (ret)
> +			goto err_out;

Can we detect the UFS device's max gear somehow? If not, the 'max-gear' 
property name doesn't sound good. Maybe calling it 'device-gear' would 
be better.

> +
> +		/* Next get the max gear supported by the UFS controller */
> +		reg = ufshcd_readl(hba, REG_UFS_PARAM0);
> +		hcd_max_gear = UFS_QCOM_MAX_GEAR(reg);
> +
> +		/*
> +		 * Now compare both the gears. If the max gear supported by the UFS device
> +		 * is compatible with UFS controller, then use the UFS device's max gear
> +		 * speed. Otherwise, use the UFS controller supported max gear speed.
> +		 */
> +		return (max_gear <= hcd_max_gear) ? max_gear : hcd_max_gear;

return max(max_gear, hcd_max_gear); ?

>   	}
>   
> +err_out:
>   	/* Default is HS-G3 */
>   	return UFS_HS_G3;
>   }
> @@ -303,7 +331,7 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>   	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>   	struct phy *phy = host->generic_phy;
>   	int ret;
> -	bool is_rate_B = UFS_QCOM_LIMIT_HS_RATE == PA_HS_MODE_B;
> +	u32 hs_gear;
>   
>   	/* Reset UFS Host Controller and PHY */
>   	ret = ufs_qcom_host_reset(hba);
> @@ -311,8 +339,9 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>   		dev_warn(hba->dev, "%s: host reset returned %d\n",
>   				  __func__, ret);
>   
> -	if (is_rate_B)
> -		phy_set_mode(phy, PHY_MODE_UFS_HS_B);
> +	/* UFS_HS_G2 is used here since that's the least gear supported by legacy Qcom platforms */
> +	hs_gear = ufs_qcom_get_hs_gear(hba, UFS_HS_G2);
> +	phy_set_mode_ext(phy, PHY_MODE_UFS_HS_B, hs_gear);
>   
>   	/* phy initialization - calibrate the phy */
>   	ret = phy_init(phy);
> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> index 214ea50acab9..c93bc52ea848 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -89,6 +89,10 @@ enum {
>   #define TMRLUT_HW_CGC_EN	BIT(6)
>   #define OCSC_HW_CGC_EN		BIT(7)
>   
> +/* bit definitions for REG_UFS_PARAM0 */
> +#define MAX_HS_GEAR_MASK	GENMASK(6, 4)
> +#define UFS_QCOM_MAX_GEAR(x)	FIELD_GET(MAX_HS_GEAR_MASK, (x))
> +
>   /* bit definition for UFS_UFS_TEST_BUS_CTRL_n */
>   #define TEST_BUS_SUB_SEL_MASK	GENMASK(4, 0)  /* All XXX_SEL fields are 5 bits wide */
>
Dmitry Baryshkov Oct. 29, 2022, 9:50 p.m. UTC | #2
On 29/10/2022 17:16, Manivannan Sadhasivam wrote:
> As done for Qcom PCIe PHY driver, let's move the register settings to the
> common qmp_phy_cfg_tables struct. This helps in adding any additional PHY
> settings needed for functionalities like HS-G4 in the future by adding one
> more instance of the qmp_phy_cfg_tables.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 223 +++++++++++++-----------
>   1 file changed, 126 insertions(+), 97 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> index c08d34ad1313..cdfda4e6d575 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> @@ -107,7 +107,7 @@ static const unsigned int sm8150_ufsphy_regs_layout[QPHY_LAYOUT_SIZE] = {
>   	[QPHY_SW_RESET]			= QPHY_V4_PCS_UFS_SW_RESET,
>   };
>   
> -static const struct qmp_phy_init_tbl msm8996_ufs_serdes_tbl[] = {
> +static const struct qmp_phy_init_tbl msm8996_ufs_serdes[] = {
>   	QMP_PHY_INIT_CFG(QSERDES_COM_CMN_CONFIG, 0x0e),
>   	QMP_PHY_INIT_CFG(QSERDES_COM_SYSCLK_EN_SEL, 0xd7),
>   	QMP_PHY_INIT_CFG(QSERDES_COM_CLK_SELECT, 0x30),
> @@ -156,12 +156,12 @@ static const struct qmp_phy_init_tbl msm8996_ufs_serdes_tbl[] = {
>   	QMP_PHY_INIT_CFG(QSERDES_COM_LOCK_CMP3_MODE1, 0x00),
>   };
>   
> -static const struct qmp_phy_init_tbl msm8996_ufs_tx_tbl[] = {
> +static const struct qmp_phy_init_tbl msm8996_ufs_tx[] = {
>   	QMP_PHY_INIT_CFG(QSERDES_TX_HIGHZ_TRANSCEIVEREN_BIAS_DRVR_EN, 0x45),
>   	QMP_PHY_INIT_CFG(QSERDES_TX_LANE_MODE, 0x02),
>   };
>   
> -static const struct qmp_phy_init_tbl msm8996_ufs_rx_tbl[] = {
> +static const struct qmp_phy_init_tbl msm8996_ufs_rx[] = {
>   	QMP_PHY_INIT_CFG(QSERDES_RX_SIGDET_LVL, 0x24),
>   	QMP_PHY_INIT_CFG(QSERDES_RX_SIGDET_CNTRL, 0x02),
>   	QMP_PHY_INIT_CFG(QSERDES_RX_RX_INTERFACE_MODE, 0x00),
> @@ -175,7 +175,7 @@ static const struct qmp_phy_init_tbl msm8996_ufs_rx_tbl[] = {
>   	QMP_PHY_INIT_CFG(QSERDES_RX_RX_EQU_ADAPTOR_CNTRL2, 0x0E),
>   };
>   
> -static const struct qmp_phy_init_tbl sm6115_ufsphy_serdes_tbl[] = {
> +static const struct qmp_phy_init_tbl sm6115_ufsphy_serdes[] = {
>   	QMP_PHY_INIT_CFG(QSERDES_COM_CMN_CONFIG, 0x0e),
>   	QMP_PHY_INIT_CFG(QSERDES_COM_SYSCLK_EN_SEL, 0x14),
>   	QMP_PHY_INIT_CFG(QSERDES_COM_CLK_SELECT, 0x30),
> @@ -231,12 +231,12 @@ static const struct qmp_phy_init_tbl sm6115_ufsphy_serdes_tbl[] = {
>   	QMP_PHY_INIT_CFG(QSERDES_COM_VCO_TUNE_MAP, 0x44),
>   };
>   
> -static const struct qmp_phy_init_tbl sm6115_ufsphy_tx_tbl[] = {
> +static const struct qmp_phy_init_tbl sm6115_ufsphy_tx[] = {
>   	QMP_PHY_INIT_CFG(QSERDES_TX_HIGHZ_TRANSCEIVEREN_BIAS_DRVR_EN, 0x45),
>   	QMP_PHY_INIT_CFG(QSERDES_TX_LANE_MODE, 0x06),
>   };
>   
> -static const struct qmp_phy_init_tbl sm6115_ufsphy_rx_tbl[] = {
> +static const struct qmp_phy_init_tbl sm6115_ufsphy_rx[] = {
>   	QMP_PHY_INIT_CFG(QSERDES_RX_SIGDET_LVL, 0x24),
>   	QMP_PHY_INIT_CFG(QSERDES_RX_SIGDET_CNTRL, 0x0F),
>   	QMP_PHY_INIT_CFG(QSERDES_RX_RX_INTERFACE_MODE, 0x40),
> @@ -254,7 +254,7 @@ static const struct qmp_phy_init_tbl sm6115_ufsphy_rx_tbl[] = {
>   	QMP_PHY_INIT_CFG(QSERDES_RX_UCDR_SO_SATURATION_AND_ENABLE, 0x5B),
>   };
>   
> -static const struct qmp_phy_init_tbl sm6115_ufsphy_pcs_tbl[] = {
> +static const struct qmp_phy_init_tbl sm6115_ufsphy_pcs[] = {
>   	QMP_PHY_INIT_CFG(QPHY_V2_PCS_RX_PWM_GEAR_BAND, 0x15),
>   	QMP_PHY_INIT_CFG(QPHY_V2_PCS_RX_SIGDET_CTRL2, 0x6d),
>   	QMP_PHY_INIT_CFG(QPHY_V2_PCS_TX_LARGE_AMP_DRV_LVL, 0x0f),
> @@ -266,7 +266,7 @@ static const struct qmp_phy_init_tbl sm6115_ufsphy_pcs_tbl[] = {
>   	QMP_PHY_INIT_CFG(QPHY_V2_PCS_RX_MIN_HIBERN8_TIME, 0x9a), /* 8 us */
>   };
>   
> -static const struct qmp_phy_init_tbl sdm845_ufsphy_serdes_tbl[] = {
> +static const struct qmp_phy_init_tbl sdm845_ufsphy_serdes[] = {
>   	QMP_PHY_INIT_CFG(QSERDES_V3_COM_SYS_CLK_CTRL, 0x02),
>   	QMP_PHY_INIT_CFG(QSERDES_V3_COM_BIAS_EN_CLKBUFLR_EN, 0x04),
>   	QMP_PHY_INIT_CFG(QSERDES_V3_COM_BG_TIMER, 0x0a),
> @@ -308,13 +308,13 @@ static const struct qmp_phy_init_tbl sdm845_ufsphy_serdes_tbl[] = {
>   	QMP_PHY_INIT_CFG(QSERDES_V3_COM_VCO_TUNE_MAP, 0x44),
>   };
>   
> -static const struct qmp_phy_init_tbl sdm845_ufsphy_tx_tbl[] = {
> +static const struct qmp_phy_init_tbl sdm845_ufsphy_tx[] = {
>   	QMP_PHY_INIT_CFG(QSERDES_V3_TX_LANE_MODE_1, 0x06),
>   	QMP_PHY_INIT_CFG(QSERDES_V3_TX_RES_CODE_LANE_OFFSET_TX, 0x04),
>   	QMP_PHY_INIT_CFG(QSERDES_V3_TX_RES_CODE_LANE_OFFSET_RX, 0x07),
>   };
>   
> -static const struct qmp_phy_init_tbl sdm845_ufsphy_rx_tbl[] = {
> +static const struct qmp_phy_init_tbl sdm845_ufsphy_rx[] = {
>   	QMP_PHY_INIT_CFG(QSERDES_V3_RX_SIGDET_LVL, 0x24),
>   	QMP_PHY_INIT_CFG(QSERDES_V3_RX_SIGDET_CNTRL, 0x0f),
>   	QMP_PHY_INIT_CFG(QSERDES_V3_RX_SIGDET_DEGLITCH_CNTRL, 0x1e),
> @@ -333,7 +333,7 @@ static const struct qmp_phy_init_tbl sdm845_ufsphy_rx_tbl[] = {
>   	QMP_PHY_INIT_CFG(QSERDES_V3_RX_RX_MODE_00, 0x59),
>   };
>   
> -static const struct qmp_phy_init_tbl sdm845_ufsphy_pcs_tbl[] = {
> +static const struct qmp_phy_init_tbl sdm845_ufsphy_pcs[] = {
>   	QMP_PHY_INIT_CFG(QPHY_V3_PCS_UFS_RX_SIGDET_CTRL2, 0x6e),
>   	QMP_PHY_INIT_CFG(QPHY_V3_PCS_UFS_TX_LARGE_AMP_DRV_LVL, 0x0a),
>   	QMP_PHY_INIT_CFG(QPHY_V3_PCS_UFS_TX_SMALL_AMP_DRV_LVL, 0x02),
> @@ -344,7 +344,7 @@ static const struct qmp_phy_init_tbl sdm845_ufsphy_pcs_tbl[] = {
>   	QMP_PHY_INIT_CFG(QPHY_V3_PCS_UFS_MULTI_LANE_CTRL1, 0x02),
>   };
>   
> -static const struct qmp_phy_init_tbl sm8150_ufsphy_serdes_tbl[] = {
> +static const struct qmp_phy_init_tbl sm8150_ufsphy_serdes[] = {
>   	QMP_PHY_INIT_CFG(QSERDES_V4_COM_SYSCLK_EN_SEL, 0xd9),
>   	QMP_PHY_INIT_CFG(QSERDES_V4_COM_HSCLK_SEL, 0x11),
>   	QMP_PHY_INIT_CFG(QSERDES_V4_COM_HSCLK_HS_SWITCH_SEL, 0x00),
> @@ -374,7 +374,7 @@ static const struct qmp_phy_init_tbl sm8150_ufsphy_serdes_tbl[] = {
>   	QMP_PHY_INIT_CFG(QSERDES_V4_COM_VCO_TUNE_MAP, 0x06),
>   };
>   
> -static const struct qmp_phy_init_tbl sm8150_ufsphy_tx_tbl[] = {
> +static const struct qmp_phy_init_tbl sm8150_ufsphy_tx[] = {
>   	QMP_PHY_INIT_CFG(QSERDES_V4_TX_PWM_GEAR_1_DIVIDER_BAND0_1, 0x06),
>   	QMP_PHY_INIT_CFG(QSERDES_V4_TX_PWM_GEAR_2_DIVIDER_BAND0_1, 0x03),
>   	QMP_PHY_INIT_CFG(QSERDES_V4_TX_PWM_GEAR_3_DIVIDER_BAND0_1, 0x01),
> @@ -383,7 +383,7 @@ static const struct qmp_phy_init_tbl sm8150_ufsphy_tx_tbl[] = {
>   	QMP_PHY_INIT_CFG(QSERDES_V4_TX_TRAN_DRVR_EMP_EN, 0x0c),
>   };
>   
> -static const struct qmp_phy_init_tbl sm8150_ufsphy_rx_tbl[] = {
> +static const struct qmp_phy_init_tbl sm8150_ufsphy_rx[] = {
>   	QMP_PHY_INIT_CFG(QSERDES_V4_RX_SIGDET_LVL, 0x24),
>   	QMP_PHY_INIT_CFG(QSERDES_V4_RX_SIGDET_CNTRL, 0x0f),
>   	QMP_PHY_INIT_CFG(QSERDES_V4_RX_SIGDET_DEGLITCH_CNTRL, 0x1e),
> @@ -421,7 +421,7 @@ static const struct qmp_phy_init_tbl sm8150_ufsphy_rx_tbl[] = {
>   
>   };
>   
> -static const struct qmp_phy_init_tbl sm8150_ufsphy_pcs_tbl[] = {
> +static const struct qmp_phy_init_tbl sm8150_ufsphy_pcs[] = {
>   	QMP_PHY_INIT_CFG(QPHY_V4_PCS_UFS_RX_SIGDET_CTRL2, 0x6d),
>   	QMP_PHY_INIT_CFG(QPHY_V4_PCS_UFS_TX_LARGE_AMP_DRV_LVL, 0x0a),
>   	QMP_PHY_INIT_CFG(QPHY_V4_PCS_UFS_TX_SMALL_AMP_DRV_LVL, 0x02),
> @@ -431,7 +431,7 @@ static const struct qmp_phy_init_tbl sm8150_ufsphy_pcs_tbl[] = {
>   	QMP_PHY_INIT_CFG(QPHY_V4_PCS_UFS_MULTI_LANE_CTRL1, 0x02),
>   };
>   
> -static const struct qmp_phy_init_tbl sm8350_ufsphy_serdes_tbl[] = {
> +static const struct qmp_phy_init_tbl sm8350_ufsphy_serdes[] = {
>   	QMP_PHY_INIT_CFG(QSERDES_V5_COM_SYSCLK_EN_SEL, 0xd9),
>   	QMP_PHY_INIT_CFG(QSERDES_V5_COM_HSCLK_SEL, 0x11),
>   	QMP_PHY_INIT_CFG(QSERDES_V5_COM_HSCLK_HS_SWITCH_SEL, 0x00),
> @@ -461,7 +461,7 @@ static const struct qmp_phy_init_tbl sm8350_ufsphy_serdes_tbl[] = {
>   	QMP_PHY_INIT_CFG(QSERDES_V5_COM_VCO_TUNE_MAP, 0x06),
>   };
>   
> -static const struct qmp_phy_init_tbl sm8350_ufsphy_tx_tbl[] = {
> +static const struct qmp_phy_init_tbl sm8350_ufsphy_tx[] = {
>   	QMP_PHY_INIT_CFG(QSERDES_V5_TX_PWM_GEAR_1_DIVIDER_BAND0_1, 0x06),
>   	QMP_PHY_INIT_CFG(QSERDES_V5_TX_PWM_GEAR_2_DIVIDER_BAND0_1, 0x03),
>   	QMP_PHY_INIT_CFG(QSERDES_V5_TX_PWM_GEAR_3_DIVIDER_BAND0_1, 0x01),
> @@ -473,7 +473,7 @@ static const struct qmp_phy_init_tbl sm8350_ufsphy_tx_tbl[] = {
>   	QMP_PHY_INIT_CFG(QSERDES_V5_TX_TRAN_DRVR_EMP_EN, 0x0c),
>   };
>   
> -static const struct qmp_phy_init_tbl sm8350_ufsphy_rx_tbl[] = {
> +static const struct qmp_phy_init_tbl sm8350_ufsphy_rx[] = {
>   	QMP_PHY_INIT_CFG(QSERDES_V5_RX_SIGDET_LVL, 0x24),
>   	QMP_PHY_INIT_CFG(QSERDES_V5_RX_SIGDET_CNTRL, 0x0f),
>   	QMP_PHY_INIT_CFG(QSERDES_V5_RX_SIGDET_DEGLITCH_CNTRL, 0x1e),
> @@ -513,7 +513,7 @@ static const struct qmp_phy_init_tbl sm8350_ufsphy_rx_tbl[] = {
>   	QMP_PHY_INIT_CFG(QSERDES_V5_RX_DCC_CTRL1, 0x0c),
>   };
>   
> -static const struct qmp_phy_init_tbl sm8350_ufsphy_pcs_tbl[] = {
> +static const struct qmp_phy_init_tbl sm8350_ufsphy_pcs[] = {
>   	QMP_PHY_INIT_CFG(QPHY_V5_PCS_UFS_RX_SIGDET_CTRL2, 0x6d),
>   	QMP_PHY_INIT_CFG(QPHY_V5_PCS_UFS_TX_LARGE_AMP_DRV_LVL, 0x0a),
>   	QMP_PHY_INIT_CFG(QPHY_V5_PCS_UFS_TX_SMALL_AMP_DRV_LVL, 0x02),
> @@ -531,19 +531,24 @@ static const struct qmp_phy_init_tbl sm8350_ufsphy_pcs_tbl[] = {
>   	QMP_PHY_INIT_CFG(QPHY_V5_PCS_UFS_MULTI_LANE_CTRL1, 0x02),
>   };
>   
> +struct qmp_phy_cfg_tables {
> +	/* Init sequence for PHY blocks - serdes, tx, rx, pcs */
> +	const struct qmp_phy_init_tbl *serdes;
> +	int serdes_num;
> +	const struct qmp_phy_init_tbl *tx;
> +	int tx_num;
> +	const struct qmp_phy_init_tbl *rx;
> +	int rx_num;
> +	const struct qmp_phy_init_tbl *pcs;
> +	int pcs_num;
> +};
> +
>   /* struct qmp_phy_cfg - per-PHY initialization config */
>   struct qmp_phy_cfg {
>   	int lanes;
>   
> -	/* Init sequence for PHY blocks - serdes, tx, rx, pcs */
> -	const struct qmp_phy_init_tbl *serdes_tbl;
> -	int serdes_tbl_num;
> -	const struct qmp_phy_init_tbl *tx_tbl;
> -	int tx_tbl_num;
> -	const struct qmp_phy_init_tbl *rx_tbl;
> -	int rx_tbl_num;
> -	const struct qmp_phy_init_tbl *pcs_tbl;
> -	int pcs_tbl_num;
> +	/* Main init sequence for PHY blocks - serdes, tx, rx, pcs */
> +	const struct qmp_phy_cfg_tables tables;
>   
>   	/* clock ids to be requested */
>   	const char * const *clk_list;
> @@ -660,12 +665,14 @@ static const char * const qmp_phy_vreg_l[] = {
>   static const struct qmp_phy_cfg msm8996_ufs_cfg = {
>   	.lanes			= 1,
>   
> -	.serdes_tbl		= msm8996_ufs_serdes_tbl,
> -	.serdes_tbl_num		= ARRAY_SIZE(msm8996_ufs_serdes_tbl),
> -	.tx_tbl			= msm8996_ufs_tx_tbl,
> -	.tx_tbl_num		= ARRAY_SIZE(msm8996_ufs_tx_tbl),
> -	.rx_tbl			= msm8996_ufs_rx_tbl,
> -	.rx_tbl_num		= ARRAY_SIZE(msm8996_ufs_rx_tbl),
> +	.tables = {
> +		.serdes		= msm8996_ufs_serdes,
> +		.serdes_num	= ARRAY_SIZE(msm8996_ufs_serdes),
> +		.tx		= msm8996_ufs_tx,
> +		.tx_num		= ARRAY_SIZE(msm8996_ufs_tx),
> +		.rx		= msm8996_ufs_rx,
> +		.rx_num		= ARRAY_SIZE(msm8996_ufs_rx),
> +	},
>   
>   	.clk_list		= msm8996_ufs_phy_clk_l,
>   	.num_clks		= ARRAY_SIZE(msm8996_ufs_phy_clk_l),
> @@ -685,14 +692,16 @@ static const struct qmp_phy_cfg msm8996_ufs_cfg = {
>   static const struct qmp_phy_cfg sdm845_ufsphy_cfg = {
>   	.lanes			= 2,
>   
> -	.serdes_tbl		= sdm845_ufsphy_serdes_tbl,
> -	.serdes_tbl_num		= ARRAY_SIZE(sdm845_ufsphy_serdes_tbl),
> -	.tx_tbl			= sdm845_ufsphy_tx_tbl,
> -	.tx_tbl_num		= ARRAY_SIZE(sdm845_ufsphy_tx_tbl),
> -	.rx_tbl			= sdm845_ufsphy_rx_tbl,
> -	.rx_tbl_num		= ARRAY_SIZE(sdm845_ufsphy_rx_tbl),
> -	.pcs_tbl		= sdm845_ufsphy_pcs_tbl,
> -	.pcs_tbl_num		= ARRAY_SIZE(sdm845_ufsphy_pcs_tbl),
> +	.tables = {
> +		.serdes		= sdm845_ufsphy_serdes,
> +		.serdes_num	= ARRAY_SIZE(sdm845_ufsphy_serdes),
> +		.tx		= sdm845_ufsphy_tx,
> +		.tx_num		= ARRAY_SIZE(sdm845_ufsphy_tx),
> +		.rx		= sdm845_ufsphy_rx,
> +		.rx_num		= ARRAY_SIZE(sdm845_ufsphy_rx),
> +		.pcs		= sdm845_ufsphy_pcs,
> +		.pcs_num	= ARRAY_SIZE(sdm845_ufsphy_pcs),
> +	},
>   	.clk_list		= sdm845_ufs_phy_clk_l,
>   	.num_clks		= ARRAY_SIZE(sdm845_ufs_phy_clk_l),
>   	.vreg_list		= qmp_phy_vreg_l,
> @@ -709,14 +718,16 @@ static const struct qmp_phy_cfg sdm845_ufsphy_cfg = {
>   static const struct qmp_phy_cfg sm6115_ufsphy_cfg = {
>   	.lanes			= 1,
>   
> -	.serdes_tbl		= sm6115_ufsphy_serdes_tbl,
> -	.serdes_tbl_num		= ARRAY_SIZE(sm6115_ufsphy_serdes_tbl),
> -	.tx_tbl			= sm6115_ufsphy_tx_tbl,
> -	.tx_tbl_num		= ARRAY_SIZE(sm6115_ufsphy_tx_tbl),
> -	.rx_tbl			= sm6115_ufsphy_rx_tbl,
> -	.rx_tbl_num		= ARRAY_SIZE(sm6115_ufsphy_rx_tbl),
> -	.pcs_tbl		= sm6115_ufsphy_pcs_tbl,
> -	.pcs_tbl_num		= ARRAY_SIZE(sm6115_ufsphy_pcs_tbl),
> +	.tables = {
> +		.serdes		= sm6115_ufsphy_serdes,
> +		.serdes_num	= ARRAY_SIZE(sm6115_ufsphy_serdes),
> +		.tx		= sm6115_ufsphy_tx,
> +		.tx_num		= ARRAY_SIZE(sm6115_ufsphy_tx),
> +		.rx		= sm6115_ufsphy_rx,
> +		.rx_num		= ARRAY_SIZE(sm6115_ufsphy_rx),
> +		.pcs		= sm6115_ufsphy_pcs,
> +		.pcs_num	= ARRAY_SIZE(sm6115_ufsphy_pcs),
> +	},
>   	.clk_list		= sdm845_ufs_phy_clk_l,
>   	.num_clks		= ARRAY_SIZE(sdm845_ufs_phy_clk_l),
>   	.vreg_list		= qmp_phy_vreg_l,
> @@ -732,14 +743,16 @@ static const struct qmp_phy_cfg sm6115_ufsphy_cfg = {
>   static const struct qmp_phy_cfg sm8150_ufsphy_cfg = {
>   	.lanes			= 2,
>   
> -	.serdes_tbl		= sm8150_ufsphy_serdes_tbl,
> -	.serdes_tbl_num		= ARRAY_SIZE(sm8150_ufsphy_serdes_tbl),
> -	.tx_tbl			= sm8150_ufsphy_tx_tbl,
> -	.tx_tbl_num		= ARRAY_SIZE(sm8150_ufsphy_tx_tbl),
> -	.rx_tbl			= sm8150_ufsphy_rx_tbl,
> -	.rx_tbl_num		= ARRAY_SIZE(sm8150_ufsphy_rx_tbl),
> -	.pcs_tbl		= sm8150_ufsphy_pcs_tbl,
> -	.pcs_tbl_num		= ARRAY_SIZE(sm8150_ufsphy_pcs_tbl),
> +	.tables = {
> +		.serdes		= sm8150_ufsphy_serdes,
> +		.serdes_num	= ARRAY_SIZE(sm8150_ufsphy_serdes),
> +		.tx		= sm8150_ufsphy_tx,
> +		.tx_num		= ARRAY_SIZE(sm8150_ufsphy_tx),
> +		.rx		= sm8150_ufsphy_rx,
> +		.rx_num		= ARRAY_SIZE(sm8150_ufsphy_rx),
> +		.pcs		= sm8150_ufsphy_pcs,
> +		.pcs_num	= ARRAY_SIZE(sm8150_ufsphy_pcs),
> +	},
>   	.clk_list		= sdm845_ufs_phy_clk_l,
>   	.num_clks		= ARRAY_SIZE(sdm845_ufs_phy_clk_l),
>   	.vreg_list		= qmp_phy_vreg_l,
> @@ -754,14 +767,16 @@ static const struct qmp_phy_cfg sm8150_ufsphy_cfg = {
>   static const struct qmp_phy_cfg sm8350_ufsphy_cfg = {
>   	.lanes			= 2,
>   
> -	.serdes_tbl		= sm8350_ufsphy_serdes_tbl,
> -	.serdes_tbl_num		= ARRAY_SIZE(sm8350_ufsphy_serdes_tbl),
> -	.tx_tbl			= sm8350_ufsphy_tx_tbl,
> -	.tx_tbl_num		= ARRAY_SIZE(sm8350_ufsphy_tx_tbl),
> -	.rx_tbl			= sm8350_ufsphy_rx_tbl,
> -	.rx_tbl_num		= ARRAY_SIZE(sm8350_ufsphy_rx_tbl),
> -	.pcs_tbl		= sm8350_ufsphy_pcs_tbl,
> -	.pcs_tbl_num		= ARRAY_SIZE(sm8350_ufsphy_pcs_tbl),
> +	.tables = {
> +		.serdes		= sm8350_ufsphy_serdes,
> +		.serdes_num	= ARRAY_SIZE(sm8350_ufsphy_serdes),
> +		.tx		= sm8350_ufsphy_tx,
> +		.tx_num		= ARRAY_SIZE(sm8350_ufsphy_tx),
> +		.rx		= sm8350_ufsphy_rx,
> +		.rx_num		= ARRAY_SIZE(sm8350_ufsphy_rx),
> +		.pcs		= sm8350_ufsphy_pcs,
> +		.pcs_num	= ARRAY_SIZE(sm8350_ufsphy_pcs),
> +	},
>   	.clk_list		= sdm845_ufs_phy_clk_l,
>   	.num_clks		= ARRAY_SIZE(sdm845_ufs_phy_clk_l),
>   	.vreg_list		= qmp_phy_vreg_l,
> @@ -776,14 +791,16 @@ static const struct qmp_phy_cfg sm8350_ufsphy_cfg = {
>   static const struct qmp_phy_cfg sm8450_ufsphy_cfg = {
>   	.lanes			= 2,
>   
> -	.serdes_tbl		= sm8350_ufsphy_serdes_tbl,
> -	.serdes_tbl_num		= ARRAY_SIZE(sm8350_ufsphy_serdes_tbl),
> -	.tx_tbl			= sm8350_ufsphy_tx_tbl,
> -	.tx_tbl_num		= ARRAY_SIZE(sm8350_ufsphy_tx_tbl),
> -	.rx_tbl			= sm8350_ufsphy_rx_tbl,
> -	.rx_tbl_num		= ARRAY_SIZE(sm8350_ufsphy_rx_tbl),
> -	.pcs_tbl		= sm8350_ufsphy_pcs_tbl,
> -	.pcs_tbl_num		= ARRAY_SIZE(sm8350_ufsphy_pcs_tbl),
> +	.tables = {
> +		.serdes		= sm8350_ufsphy_serdes,
> +		.serdes_num	= ARRAY_SIZE(sm8350_ufsphy_serdes),
> +		.tx		= sm8350_ufsphy_tx,
> +		.tx_num		= ARRAY_SIZE(sm8350_ufsphy_tx),
> +		.rx		= sm8350_ufsphy_rx,
> +		.rx_num		= ARRAY_SIZE(sm8350_ufsphy_rx),
> +		.pcs		= sm8350_ufsphy_pcs,
> +		.pcs_num	= ARRAY_SIZE(sm8350_ufsphy_pcs),
> +	},
>   	.clk_list		= sm8450_ufs_phy_clk_l,
>   	.num_clks		= ARRAY_SIZE(sm8450_ufs_phy_clk_l),
>   	.vreg_list		= qmp_phy_vreg_l,
> @@ -826,16 +843,43 @@ static void qmp_ufs_configure(void __iomem *base,
>   	qmp_ufs_configure_lane(base, regs, tbl, num, 0xff);
>   }
>   
> -static int qmp_ufs_serdes_init(struct qmp_phy *qphy)
> +static void qmp_ufs_serdes_init(struct qmp_phy *qphy, const struct qmp_phy_cfg_tables *tables)
>   {
>   	const struct qmp_phy_cfg *cfg = qphy->cfg;
>   	void __iomem *serdes = qphy->serdes;
> -	const struct qmp_phy_init_tbl *serdes_tbl = cfg->serdes_tbl;
> -	int serdes_tbl_num = cfg->serdes_tbl_num;
>   
> -	qmp_ufs_configure(serdes, cfg->regs, serdes_tbl, serdes_tbl_num);
> +	if (!tables)
> +		return;
>   
> -	return 0;
> +	qmp_ufs_configure(serdes, cfg->regs, tables->serdes, tables->serdes_num);
> +}
> +
> +static void qmp_ufs_lanes_init(struct qmp_phy *qphy, const struct qmp_phy_cfg_tables *tables)
> +{
> +	const struct qmp_phy_cfg *cfg = qphy->cfg;
> +	void __iomem *tx = qphy->tx;
> +	void __iomem *rx = qphy->rx;
> +
> +	qmp_ufs_configure_lane(tx, cfg->regs, tables->tx, tables->tx_num, 1);
> +
> +	if (cfg->lanes >= 2)
> +		qmp_ufs_configure_lane(qphy->tx2, cfg->regs, tables->tx, tables->tx_num, 2);
> +
> +	qmp_ufs_configure_lane(rx, cfg->regs, tables->rx, tables->rx_num, 1);
> +
> +	if (cfg->lanes >= 2)
> +		qmp_ufs_configure_lane(qphy->rx2, cfg->regs, tables->rx, tables->rx_num, 2);
> +}
> +
> +static void qmp_ufs_pcs_init(struct qmp_phy *qphy, const struct qmp_phy_cfg_tables *tables)
> +{
> +	const struct qmp_phy_cfg *cfg = qphy->cfg;
> +	void __iomem *pcs = qphy->pcs;
> +
> +	if (!tables)
> +		return;
> +
> +	qmp_ufs_configure(pcs, cfg->regs, tables->pcs, tables->pcs_num);
>   }
>   
>   static int qmp_ufs_com_init(struct qmp_phy *qphy)
> @@ -933,31 +977,16 @@ static int qmp_ufs_power_on(struct phy *phy)
>   	struct qmp_phy *qphy = phy_get_drvdata(phy);
>   	struct qcom_qmp *qmp = qphy->qmp;
>   	const struct qmp_phy_cfg *cfg = qphy->cfg;
> -	void __iomem *tx = qphy->tx;
> -	void __iomem *rx = qphy->rx;
>   	void __iomem *pcs = qphy->pcs;
>   	void __iomem *status;
>   	unsigned int mask, val, ready;
>   	int ret;
>   
> -	qmp_ufs_serdes_init(qphy);
> -
> -	/* Tx, Rx, and PCS configurations */
> -	qmp_ufs_configure_lane(tx, cfg->regs, cfg->tx_tbl, cfg->tx_tbl_num, 1);
> +	qmp_ufs_serdes_init(qphy, &cfg->tables);
>   
> -	if (cfg->lanes >= 2) {
> -		qmp_ufs_configure_lane(qphy->tx2, cfg->regs,
> -					cfg->tx_tbl, cfg->tx_tbl_num, 2);
> -	}
> -
> -	qmp_ufs_configure_lane(rx, cfg->regs, cfg->rx_tbl, cfg->rx_tbl_num, 1);
> -
> -	if (cfg->lanes >= 2) {
> -		qmp_ufs_configure_lane(qphy->rx2, cfg->regs,
> -					cfg->rx_tbl, cfg->rx_tbl_num, 2);
> -	}
> +	qmp_ufs_lanes_init(qphy, &cfg->tables);
>   
> -	qmp_ufs_configure(pcs, cfg->regs, cfg->pcs_tbl, cfg->pcs_tbl_num);
> +	qmp_ufs_pcs_init(qphy, &cfg->tables);

I'd suggest going straight to qmp_ufs_init_registers, which would 
contain both serdes, lanes and pcs inits.

>   
>   	ret = reset_control_deassert(qmp->ufs_reset);
>   	if (ret)
Dmitry Baryshkov Oct. 29, 2022, 9:54 p.m. UTC | #3
On 29/10/2022 17:16, Manivannan Sadhasivam wrote:
> Add separate tables_hs_g4 instance to allow the PHY driver to configure the
> PHY in HS G4 mode. The individual SoC configs need to supply the Rx, Tx and
> PCS register setting in tables_hs_g4 and the UFS driver can request the
> Hs G4 mode by calling phy_set_mode_ext() with submode set to UFS_HS_G4.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

(Especially if changed to qmp_ufs_init_registers()).

> ---
>   drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> index 4c6a2b5afc9a..5f2a012707b7 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> @@ -553,6 +553,8 @@ struct qmp_phy_cfg {
>   	const struct qmp_phy_cfg_tables tables;
>   	/* Additional sequence for HS Series B */
>   	const struct qmp_phy_cfg_tables tables_hs_b;
> +	/* Additional sequence for HS G4 */
> +	const struct qmp_phy_cfg_tables tables_hs_g4;
>   
>   	/* clock ids to be requested */
>   	const char * const *clk_list;
> @@ -587,6 +589,7 @@ struct qmp_phy_cfg {
>    * @pcs_misc: iomapped memory space for lane's pcs_misc
>    * @qmp: QMP phy to which this lane belongs
>    * @mode: PHY mode configured by the UFS driver
> + * @submode: PHY submode configured by the UFS driver
>    */
>   struct qmp_phy {
>   	struct phy *phy;
> @@ -600,6 +603,7 @@ struct qmp_phy {
>   	void __iomem *pcs_misc;
>   	struct qcom_qmp *qmp;
>   	u32 mode;
> +	u32 submode;
>   };
>   
>   /**
> @@ -993,8 +997,12 @@ static int qmp_ufs_power_on(struct phy *phy)
>   		qmp_ufs_serdes_init(qphy, &cfg->tables_hs_b);
>   
>   	qmp_ufs_lanes_init(qphy, &cfg->tables);
> +	if (qphy->submode == UFS_HS_G4)
> +		qmp_ufs_lanes_init(qphy, &cfg->tables_hs_g4);
>   
>   	qmp_ufs_pcs_init(qphy, &cfg->tables);
> +	if (qphy->submode == UFS_HS_G4)
> +		qmp_ufs_pcs_init(qphy, &cfg->tables_hs_g4);
>   
>   	ret = reset_control_deassert(qmp->ufs_reset);
>   	if (ret)
> @@ -1083,6 +1091,7 @@ static int qmp_ufs_set_mode(struct phy *phy, enum phy_mode mode, int submode)
>   	struct qmp_phy *qphy = phy_get_drvdata(phy);
>   
>   	qphy->mode = mode;
> +	qphy->submode = submode;
>   
>   	return 0;
>   }
Dmitry Baryshkov Oct. 29, 2022, 9:55 p.m. UTC | #4
On 29/10/2022 17:16, Manivannan Sadhasivam wrote:
> Since now there is support for configuring the HS Rate B mode properly,
> let's move the register setting to tables_hs_b struct for all SoCs.
> 
> This allows the PHY to be configured in Rate A initially and then in
> Rate B if requested by the UFS driver.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Dmitry Baryshkov Oct. 29, 2022, 9:56 p.m. UTC | #5
On 29/10/2022 17:16, Manivannan Sadhasivam wrote:
> goto in error path is useful if the function needs to do cleanup other
> than returning the error code. But in this driver, goto statements are
> used for just returning the error code in many places. This really
> makes it hard to read the code.
> 
> So let's get rid of those goto statements and just return the error code
> directly.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   drivers/ufs/host/ufs-qcom.c | 100 +++++++++++++++---------------------
>   1 file changed, 41 insertions(+), 59 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Dmitry Baryshkov Oct. 29, 2022, 9:58 p.m. UTC | #6
On 29/10/2022 17:16, Manivannan Sadhasivam wrote:
> Use bitfield macros where appropriate to simplify the driver.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   drivers/ufs/host/ufs-qcom.h | 58 ++++++++++++++++---------------------
>   1 file changed, 25 insertions(+), 33 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> index 44466a395bb5..6cb0776456b3 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -17,12 +17,9 @@
>   #define DEFAULT_CLK_RATE_HZ     1000000
>   #define BUS_VECTOR_NAME_LEN     32
>   
> -#define UFS_HW_VER_MAJOR_SHFT	(28)
> -#define UFS_HW_VER_MAJOR_MASK	(0x000F << UFS_HW_VER_MAJOR_SHFT)
> -#define UFS_HW_VER_MINOR_SHFT	(16)
> -#define UFS_HW_VER_MINOR_MASK	(0x0FFF << UFS_HW_VER_MINOR_SHFT)
> -#define UFS_HW_VER_STEP_SHFT	(0)
> -#define UFS_HW_VER_STEP_MASK	(0xFFFF << UFS_HW_VER_STEP_SHFT)
> +#define UFS_HW_VER_MAJOR_MASK	GENMASK(31, 28)
> +#define UFS_HW_VER_MINOR_MASK	GENMASK(27, 16)
> +#define UFS_HW_VER_STEP_MASK	GENMASK(15, 0)
>   
>   /* vendor specific pre-defined parameters */
>   #define SLOW 1
> @@ -76,24 +73,24 @@ enum {
>   #define UFS_CNTLR_3_x_x_VEN_REGS_OFFSET(x)	(0x400 + x)
>   
>   /* bit definitions for REG_UFS_CFG1 register */
> -#define QUNIPRO_SEL		0x1
> -#define UTP_DBG_RAMS_EN		0x20000
> +#define QUNIPRO_SEL		BIT(0)
> +#define UTP_DBG_RAMS_EN		BIT(17)
>   #define TEST_BUS_EN		BIT(18)
>   #define TEST_BUS_SEL		GENMASK(22, 19)
>   #define UFS_REG_TEST_BUS_EN	BIT(30)
>   
>   /* bit definitions for REG_UFS_CFG2 register */
> -#define UAWM_HW_CGC_EN		(1 << 0)
> -#define UARM_HW_CGC_EN		(1 << 1)
> -#define TXUC_HW_CGC_EN		(1 << 2)
> -#define RXUC_HW_CGC_EN		(1 << 3)
> -#define DFC_HW_CGC_EN		(1 << 4)
> -#define TRLUT_HW_CGC_EN		(1 << 5)
> -#define TMRLUT_HW_CGC_EN	(1 << 6)
> -#define OCSC_HW_CGC_EN		(1 << 7)
> +#define UAWM_HW_CGC_EN		BIT(0)
> +#define UARM_HW_CGC_EN		BIT(1)
> +#define TXUC_HW_CGC_EN		BIT(2)
> +#define RXUC_HW_CGC_EN		BIT(3)
> +#define DFC_HW_CGC_EN		BIT(4)
> +#define TRLUT_HW_CGC_EN		BIT(5)
> +#define TMRLUT_HW_CGC_EN	BIT(6)
> +#define OCSC_HW_CGC_EN		BIT(7)
>   
>   /* bit definition for UFS_UFS_TEST_BUS_CTRL_n */
> -#define TEST_BUS_SUB_SEL_MASK	0x1F  /* All XXX_SEL fields are 5 bits wide */
> +#define TEST_BUS_SUB_SEL_MASK	GENMASK(4, 0)  /* All XXX_SEL fields are 5 bits wide */
>   
>   #define REG_UFS_CFG2_CGC_EN_ALL (UAWM_HW_CGC_EN | UARM_HW_CGC_EN |\
>   				 TXUC_HW_CGC_EN | RXUC_HW_CGC_EN |\
> @@ -101,17 +98,12 @@ enum {
>   				 TMRLUT_HW_CGC_EN | OCSC_HW_CGC_EN)
>   
>   /* bit offset */
> -enum {
> -	OFFSET_UFS_PHY_SOFT_RESET           = 1,
> -	OFFSET_CLK_NS_REG                   = 10,
> -};
> +#define OFFSET_CLK_NS_REG		0xa
>   
>   /* bit masks */
> -enum {
> -	MASK_UFS_PHY_SOFT_RESET             = 0x2,
> -	MASK_TX_SYMBOL_CLK_1US_REG          = 0x3FF,
> -	MASK_CLK_NS_REG                     = 0xFFFC00,
> -};
> +#define MASK_UFS_PHY_SOFT_RESET		BIT(1)
> +#define MASK_TX_SYMBOL_CLK_1US_REG	GENMASK(9, 0)
> +#define MASK_CLK_NS_REG			GENMASK(23, 10)
>   
>   /* QCOM UFS debug print bit mask */
>   #define UFS_QCOM_DBG_PRINT_REGS_EN	BIT(0)
> @@ -135,15 +127,15 @@ ufs_qcom_get_controller_revision(struct ufs_hba *hba,
>   {
>   	u32 ver = ufshcd_readl(hba, REG_UFS_HW_VERSION);
>   
> -	*major = (ver & UFS_HW_VER_MAJOR_MASK) >> UFS_HW_VER_MAJOR_SHFT;
> -	*minor = (ver & UFS_HW_VER_MINOR_MASK) >> UFS_HW_VER_MINOR_SHFT;
> -	*step = (ver & UFS_HW_VER_STEP_MASK) >> UFS_HW_VER_STEP_SHFT;
> +	*major = FIELD_GET(UFS_HW_VER_MAJOR_MASK, ver);
> +	*minor = FIELD_GET(UFS_HW_VER_MINOR_MASK, ver);
> +	*step = FIELD_GET(UFS_HW_VER_STEP_MASK, ver);
>   };
>   
>   static inline void ufs_qcom_assert_reset(struct ufs_hba *hba)
>   {
> -	ufshcd_rmwl(hba, MASK_UFS_PHY_SOFT_RESET,
> -			1 << OFFSET_UFS_PHY_SOFT_RESET, REG_UFS_CFG1);
> +	ufshcd_rmwl(hba, MASK_UFS_PHY_SOFT_RESET, FIELD_PREP(MASK_UFS_PHY_SOFT_RESET, 1),

Nit: I'd just define the value too and use the defined name here.

> +		    REG_UFS_CFG1);
>   
>   	/*
>   	 * Make sure assertion of ufs phy reset is written to
> @@ -154,8 +146,8 @@ static inline void ufs_qcom_assert_reset(struct ufs_hba *hba)
>   
>   static inline void ufs_qcom_deassert_reset(struct ufs_hba *hba)
>   {
> -	ufshcd_rmwl(hba, MASK_UFS_PHY_SOFT_RESET,
> -			0 << OFFSET_UFS_PHY_SOFT_RESET, REG_UFS_CFG1);
> +	ufshcd_rmwl(hba, MASK_UFS_PHY_SOFT_RESET, FIELD_PREP(MASK_UFS_PHY_SOFT_RESET, 0),

Nit: FIELD_PREP is always 0.

> +		    REG_UFS_CFG1);
>   
>   	/*
>   	 * Make sure de-assertion of ufs phy reset is written to
Dmitry Baryshkov Oct. 29, 2022, 10:06 p.m. UTC | #7
On 29/10/2022 17:16, Manivannan Sadhasivam wrote:
> Fix the register name used for offset 0xD0. The correct name is
> REG_UFS_PARAM0.

The vendor kernels starting from 3.10 define this register as 
RETRY_TIMER_REG (but it is unused). I'd suggest adding a comment about 
the older register name.

> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   drivers/ufs/host/ufs-qcom.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> index 6cb0776456b3..214ea50acab9 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -33,7 +33,7 @@ enum {
>   	REG_UFS_TX_SYMBOL_CLK_NS_US         = 0xC4,
>   	REG_UFS_LOCAL_PORT_ID_REG           = 0xC8,
>   	REG_UFS_PA_ERR_CODE                 = 0xCC,
> -	REG_UFS_RETRY_TIMER_REG             = 0xD0,
> +	REG_UFS_PARAM0                      = 0xD0,
>   	REG_UFS_PA_LINK_STARTUP_TIMER       = 0xD8,
>   	REG_UFS_CFG1                        = 0xDC,
>   	REG_UFS_CFG2                        = 0xE0,
Manivannan Sadhasivam Oct. 31, 2022, 2:50 p.m. UTC | #8
On Sun, Oct 30, 2022 at 12:58:57AM +0300, Dmitry Baryshkov wrote:
> On 29/10/2022 17:16, Manivannan Sadhasivam wrote:
> > Use bitfield macros where appropriate to simplify the driver.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >   drivers/ufs/host/ufs-qcom.h | 58 ++++++++++++++++---------------------
> >   1 file changed, 25 insertions(+), 33 deletions(-)
> > 
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> > diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> > index 44466a395bb5..6cb0776456b3 100644
> > --- a/drivers/ufs/host/ufs-qcom.h
> > +++ b/drivers/ufs/host/ufs-qcom.h
> > @@ -17,12 +17,9 @@
> >   #define DEFAULT_CLK_RATE_HZ     1000000
> >   #define BUS_VECTOR_NAME_LEN     32
> > -#define UFS_HW_VER_MAJOR_SHFT	(28)
> > -#define UFS_HW_VER_MAJOR_MASK	(0x000F << UFS_HW_VER_MAJOR_SHFT)
> > -#define UFS_HW_VER_MINOR_SHFT	(16)
> > -#define UFS_HW_VER_MINOR_MASK	(0x0FFF << UFS_HW_VER_MINOR_SHFT)
> > -#define UFS_HW_VER_STEP_SHFT	(0)
> > -#define UFS_HW_VER_STEP_MASK	(0xFFFF << UFS_HW_VER_STEP_SHFT)
> > +#define UFS_HW_VER_MAJOR_MASK	GENMASK(31, 28)
> > +#define UFS_HW_VER_MINOR_MASK	GENMASK(27, 16)
> > +#define UFS_HW_VER_STEP_MASK	GENMASK(15, 0)
> >   /* vendor specific pre-defined parameters */
> >   #define SLOW 1
> > @@ -76,24 +73,24 @@ enum {
> >   #define UFS_CNTLR_3_x_x_VEN_REGS_OFFSET(x)	(0x400 + x)
> >   /* bit definitions for REG_UFS_CFG1 register */
> > -#define QUNIPRO_SEL		0x1
> > -#define UTP_DBG_RAMS_EN		0x20000
> > +#define QUNIPRO_SEL		BIT(0)
> > +#define UTP_DBG_RAMS_EN		BIT(17)
> >   #define TEST_BUS_EN		BIT(18)
> >   #define TEST_BUS_SEL		GENMASK(22, 19)
> >   #define UFS_REG_TEST_BUS_EN	BIT(30)
> >   /* bit definitions for REG_UFS_CFG2 register */
> > -#define UAWM_HW_CGC_EN		(1 << 0)
> > -#define UARM_HW_CGC_EN		(1 << 1)
> > -#define TXUC_HW_CGC_EN		(1 << 2)
> > -#define RXUC_HW_CGC_EN		(1 << 3)
> > -#define DFC_HW_CGC_EN		(1 << 4)
> > -#define TRLUT_HW_CGC_EN		(1 << 5)
> > -#define TMRLUT_HW_CGC_EN	(1 << 6)
> > -#define OCSC_HW_CGC_EN		(1 << 7)
> > +#define UAWM_HW_CGC_EN		BIT(0)
> > +#define UARM_HW_CGC_EN		BIT(1)
> > +#define TXUC_HW_CGC_EN		BIT(2)
> > +#define RXUC_HW_CGC_EN		BIT(3)
> > +#define DFC_HW_CGC_EN		BIT(4)
> > +#define TRLUT_HW_CGC_EN		BIT(5)
> > +#define TMRLUT_HW_CGC_EN	BIT(6)
> > +#define OCSC_HW_CGC_EN		BIT(7)
> >   /* bit definition for UFS_UFS_TEST_BUS_CTRL_n */
> > -#define TEST_BUS_SUB_SEL_MASK	0x1F  /* All XXX_SEL fields are 5 bits wide */
> > +#define TEST_BUS_SUB_SEL_MASK	GENMASK(4, 0)  /* All XXX_SEL fields are 5 bits wide */
> >   #define REG_UFS_CFG2_CGC_EN_ALL (UAWM_HW_CGC_EN | UARM_HW_CGC_EN |\
> >   				 TXUC_HW_CGC_EN | RXUC_HW_CGC_EN |\
> > @@ -101,17 +98,12 @@ enum {
> >   				 TMRLUT_HW_CGC_EN | OCSC_HW_CGC_EN)
> >   /* bit offset */
> > -enum {
> > -	OFFSET_UFS_PHY_SOFT_RESET           = 1,
> > -	OFFSET_CLK_NS_REG                   = 10,
> > -};
> > +#define OFFSET_CLK_NS_REG		0xa
> >   /* bit masks */
> > -enum {
> > -	MASK_UFS_PHY_SOFT_RESET             = 0x2,
> > -	MASK_TX_SYMBOL_CLK_1US_REG          = 0x3FF,
> > -	MASK_CLK_NS_REG                     = 0xFFFC00,
> > -};
> > +#define MASK_UFS_PHY_SOFT_RESET		BIT(1)
> > +#define MASK_TX_SYMBOL_CLK_1US_REG	GENMASK(9, 0)
> > +#define MASK_CLK_NS_REG			GENMASK(23, 10)
> >   /* QCOM UFS debug print bit mask */
> >   #define UFS_QCOM_DBG_PRINT_REGS_EN	BIT(0)
> > @@ -135,15 +127,15 @@ ufs_qcom_get_controller_revision(struct ufs_hba *hba,
> >   {
> >   	u32 ver = ufshcd_readl(hba, REG_UFS_HW_VERSION);
> > -	*major = (ver & UFS_HW_VER_MAJOR_MASK) >> UFS_HW_VER_MAJOR_SHFT;
> > -	*minor = (ver & UFS_HW_VER_MINOR_MASK) >> UFS_HW_VER_MINOR_SHFT;
> > -	*step = (ver & UFS_HW_VER_STEP_MASK) >> UFS_HW_VER_STEP_SHFT;
> > +	*major = FIELD_GET(UFS_HW_VER_MAJOR_MASK, ver);
> > +	*minor = FIELD_GET(UFS_HW_VER_MINOR_MASK, ver);
> > +	*step = FIELD_GET(UFS_HW_VER_STEP_MASK, ver);
> >   };
> >   static inline void ufs_qcom_assert_reset(struct ufs_hba *hba)
> >   {
> > -	ufshcd_rmwl(hba, MASK_UFS_PHY_SOFT_RESET,
> > -			1 << OFFSET_UFS_PHY_SOFT_RESET, REG_UFS_CFG1);
> > +	ufshcd_rmwl(hba, MASK_UFS_PHY_SOFT_RESET, FIELD_PREP(MASK_UFS_PHY_SOFT_RESET, 1),
> 
> Nit: I'd just define the value too and use the defined name here.
> 
> > +		    REG_UFS_CFG1);
> >   	/*
> >   	 * Make sure assertion of ufs phy reset is written to
> > @@ -154,8 +146,8 @@ static inline void ufs_qcom_assert_reset(struct ufs_hba *hba)
> >   static inline void ufs_qcom_deassert_reset(struct ufs_hba *hba)
> >   {
> > -	ufshcd_rmwl(hba, MASK_UFS_PHY_SOFT_RESET,
> > -			0 << OFFSET_UFS_PHY_SOFT_RESET, REG_UFS_CFG1);
> > +	ufshcd_rmwl(hba, MASK_UFS_PHY_SOFT_RESET, FIELD_PREP(MASK_UFS_PHY_SOFT_RESET, 0),
> 
> Nit: FIELD_PREP is always 0.
> 

I know but this make the code in sync with reset assert.

Thanks,
Mani

> > +		    REG_UFS_CFG1);
> >   	/*
> >   	 * Make sure de-assertion of ufs phy reset is written to
> 
> -- 
> With best wishes
> Dmitry
>
Manivannan Sadhasivam Oct. 31, 2022, 3:39 p.m. UTC | #9
On Sun, Oct 30, 2022 at 12:48:21AM +0300, Dmitry Baryshkov wrote:
> On 29/10/2022 17:16, Manivannan Sadhasivam wrote:
> > Starting from UFS controller v4, Qcom supports dual gear mode (i.e., the
> > controller/PHY can be configured to run in two gear speeds). But that
> > requires an agreement between the UFS controller and the UFS device.
> > This commit finds the max gear supported by both controller and device
> > then decides which one to use.
> > 
> > UFS controller's max gear can be read from the REG_UFS_PARAM0 register and
> > UFS device's max gear can be read from the "max-gear" devicetree property.
> > 
> > The UFS PHY also needs to be configured with the decided gear using the
> > phy_set_mode_ext() API.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >   drivers/ufs/host/ufs-qcom.c | 35 ++++++++++++++++++++++++++++++++---
> >   drivers/ufs/host/ufs-qcom.h |  4 ++++
> >   2 files changed, 36 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > index f952cc76919f..268463e92d67 100644
> > --- a/drivers/ufs/host/ufs-qcom.c
> > +++ b/drivers/ufs/host/ufs-qcom.c
> > @@ -281,6 +281,9 @@ static int ufs_qcom_host_reset(struct ufs_hba *hba)
> >   static u32 ufs_qcom_get_hs_gear(struct ufs_hba *hba, u32 hs_gear)
> >   {
> >   	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> > +	struct device *dev = hba->dev;
> > +	u32 max_gear, hcd_max_gear, reg;
> > +	int ret;
> >   	if (host->hw_ver.major == 0x1) {
> >   		/*
> > @@ -292,8 +295,33 @@ static u32 ufs_qcom_get_hs_gear(struct ufs_hba *hba, u32 hs_gear)
> >   		 */
> >   		if (hs_gear > UFS_HS_G2)
> >   			return UFS_HS_G2;
> > +	} else if (host->hw_ver.major > 0x3) {
> > +		/*
> > +		 * Starting from UFS controller v4, Qcom supports dual gear mode (i.e., the
> > +		 * controller/PHY can be configured to run in two gear speeds). But that
> > +		 * requires an agreement between the UFS controller and the device. Below
> > +		 * code tries to find the max gear of both and decides which gear to use.
> > +		 *
> > +		 * First get the max gear supported by the UFS device if available.
> > +		 * If the property is not defined in devicetree, then use the default gear.
> > +		 */
> > +		ret = of_property_read_u32(dev->of_node, "max-gear", &max_gear);
> > +		if (ret)
> > +			goto err_out;
> 
> Can we detect the UFS device's max gear somehow? If not, the 'max-gear'
> property name doesn't sound good. Maybe calling it 'device-gear' would be
> better.
> 
> > +
> > +		/* Next get the max gear supported by the UFS controller */
> > +		reg = ufshcd_readl(hba, REG_UFS_PARAM0);
> > +		hcd_max_gear = UFS_QCOM_MAX_GEAR(reg);
> > +
> > +		/*
> > +		 * Now compare both the gears. If the max gear supported by the UFS device
> > +		 * is compatible with UFS controller, then use the UFS device's max gear
> > +		 * speed. Otherwise, use the UFS controller supported max gear speed.
> > +		 */
> > +		return (max_gear <= hcd_max_gear) ? max_gear : hcd_max_gear;
> 
> return max(max_gear, hcd_max_gear); ?
> 

min() should work...

Thanks,
Mani

> >   	}
> > +err_out:
> >   	/* Default is HS-G3 */
> >   	return UFS_HS_G3;
> >   }
> > @@ -303,7 +331,7 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
> >   	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> >   	struct phy *phy = host->generic_phy;
> >   	int ret;
> > -	bool is_rate_B = UFS_QCOM_LIMIT_HS_RATE == PA_HS_MODE_B;
> > +	u32 hs_gear;
> >   	/* Reset UFS Host Controller and PHY */
> >   	ret = ufs_qcom_host_reset(hba);
> > @@ -311,8 +339,9 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
> >   		dev_warn(hba->dev, "%s: host reset returned %d\n",
> >   				  __func__, ret);
> > -	if (is_rate_B)
> > -		phy_set_mode(phy, PHY_MODE_UFS_HS_B);
> > +	/* UFS_HS_G2 is used here since that's the least gear supported by legacy Qcom platforms */
> > +	hs_gear = ufs_qcom_get_hs_gear(hba, UFS_HS_G2);
> > +	phy_set_mode_ext(phy, PHY_MODE_UFS_HS_B, hs_gear);
> >   	/* phy initialization - calibrate the phy */
> >   	ret = phy_init(phy);
> > diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> > index 214ea50acab9..c93bc52ea848 100644
> > --- a/drivers/ufs/host/ufs-qcom.h
> > +++ b/drivers/ufs/host/ufs-qcom.h
> > @@ -89,6 +89,10 @@ enum {
> >   #define TMRLUT_HW_CGC_EN	BIT(6)
> >   #define OCSC_HW_CGC_EN		BIT(7)
> > +/* bit definitions for REG_UFS_PARAM0 */
> > +#define MAX_HS_GEAR_MASK	GENMASK(6, 4)
> > +#define UFS_QCOM_MAX_GEAR(x)	FIELD_GET(MAX_HS_GEAR_MASK, (x))
> > +
> >   /* bit definition for UFS_UFS_TEST_BUS_CTRL_n */
> >   #define TEST_BUS_SUB_SEL_MASK	GENMASK(4, 0)  /* All XXX_SEL fields are 5 bits wide */
> 
> -- 
> With best wishes
> Dmitry
>
Manivannan Sadhasivam Oct. 31, 2022, 3:46 p.m. UTC | #10
On Sun, Oct 30, 2022 at 12:50:50AM +0300, Dmitry Baryshkov wrote:
> On 29/10/2022 17:16, Manivannan Sadhasivam wrote:
> > As done for Qcom PCIe PHY driver, let's move the register settings to the
> > common qmp_phy_cfg_tables struct. This helps in adding any additional PHY
> > settings needed for functionalities like HS-G4 in the future by adding one
> > more instance of the qmp_phy_cfg_tables.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >   drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 223 +++++++++++++-----------
> >   1 file changed, 126 insertions(+), 97 deletions(-)
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> > index c08d34ad1313..cdfda4e6d575 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> > @@ -107,7 +107,7 @@ static const unsigned int sm8150_ufsphy_regs_layout[QPHY_LAYOUT_SIZE] = {
> >   	[QPHY_SW_RESET]			= QPHY_V4_PCS_UFS_SW_RESET,
> >   };
> > -static const struct qmp_phy_init_tbl msm8996_ufs_serdes_tbl[] = {
> > +static const struct qmp_phy_init_tbl msm8996_ufs_serdes[] = {
> >   	QMP_PHY_INIT_CFG(QSERDES_COM_CMN_CONFIG, 0x0e),
> >   	QMP_PHY_INIT_CFG(QSERDES_COM_SYSCLK_EN_SEL, 0xd7),
> >   	QMP_PHY_INIT_CFG(QSERDES_COM_CLK_SELECT, 0x30),
> > @@ -156,12 +156,12 @@ static const struct qmp_phy_init_tbl msm8996_ufs_serdes_tbl[] = {
> >   	QMP_PHY_INIT_CFG(QSERDES_COM_LOCK_CMP3_MODE1, 0x00),
> >   };
> > -static const struct qmp_phy_init_tbl msm8996_ufs_tx_tbl[] = {
> > +static const struct qmp_phy_init_tbl msm8996_ufs_tx[] = {
> >   	QMP_PHY_INIT_CFG(QSERDES_TX_HIGHZ_TRANSCEIVEREN_BIAS_DRVR_EN, 0x45),
> >   	QMP_PHY_INIT_CFG(QSERDES_TX_LANE_MODE, 0x02),
> >   };
> > -static const struct qmp_phy_init_tbl msm8996_ufs_rx_tbl[] = {
> > +static const struct qmp_phy_init_tbl msm8996_ufs_rx[] = {
> >   	QMP_PHY_INIT_CFG(QSERDES_RX_SIGDET_LVL, 0x24),
> >   	QMP_PHY_INIT_CFG(QSERDES_RX_SIGDET_CNTRL, 0x02),
> >   	QMP_PHY_INIT_CFG(QSERDES_RX_RX_INTERFACE_MODE, 0x00),
> > @@ -175,7 +175,7 @@ static const struct qmp_phy_init_tbl msm8996_ufs_rx_tbl[] = {
> >   	QMP_PHY_INIT_CFG(QSERDES_RX_RX_EQU_ADAPTOR_CNTRL2, 0x0E),
> >   };
> > -static const struct qmp_phy_init_tbl sm6115_ufsphy_serdes_tbl[] = {
> > +static const struct qmp_phy_init_tbl sm6115_ufsphy_serdes[] = {
> >   	QMP_PHY_INIT_CFG(QSERDES_COM_CMN_CONFIG, 0x0e),
> >   	QMP_PHY_INIT_CFG(QSERDES_COM_SYSCLK_EN_SEL, 0x14),
> >   	QMP_PHY_INIT_CFG(QSERDES_COM_CLK_SELECT, 0x30),
> > @@ -231,12 +231,12 @@ static const struct qmp_phy_init_tbl sm6115_ufsphy_serdes_tbl[] = {
> >   	QMP_PHY_INIT_CFG(QSERDES_COM_VCO_TUNE_MAP, 0x44),
> >   };
> > -static const struct qmp_phy_init_tbl sm6115_ufsphy_tx_tbl[] = {
> > +static const struct qmp_phy_init_tbl sm6115_ufsphy_tx[] = {
> >   	QMP_PHY_INIT_CFG(QSERDES_TX_HIGHZ_TRANSCEIVEREN_BIAS_DRVR_EN, 0x45),
> >   	QMP_PHY_INIT_CFG(QSERDES_TX_LANE_MODE, 0x06),
> >   };
> > -static const struct qmp_phy_init_tbl sm6115_ufsphy_rx_tbl[] = {
> > +static const struct qmp_phy_init_tbl sm6115_ufsphy_rx[] = {
> >   	QMP_PHY_INIT_CFG(QSERDES_RX_SIGDET_LVL, 0x24),
> >   	QMP_PHY_INIT_CFG(QSERDES_RX_SIGDET_CNTRL, 0x0F),
> >   	QMP_PHY_INIT_CFG(QSERDES_RX_RX_INTERFACE_MODE, 0x40),
> > @@ -254,7 +254,7 @@ static const struct qmp_phy_init_tbl sm6115_ufsphy_rx_tbl[] = {
> >   	QMP_PHY_INIT_CFG(QSERDES_RX_UCDR_SO_SATURATION_AND_ENABLE, 0x5B),
> >   };
> > -static const struct qmp_phy_init_tbl sm6115_ufsphy_pcs_tbl[] = {
> > +static const struct qmp_phy_init_tbl sm6115_ufsphy_pcs[] = {
> >   	QMP_PHY_INIT_CFG(QPHY_V2_PCS_RX_PWM_GEAR_BAND, 0x15),
> >   	QMP_PHY_INIT_CFG(QPHY_V2_PCS_RX_SIGDET_CTRL2, 0x6d),
> >   	QMP_PHY_INIT_CFG(QPHY_V2_PCS_TX_LARGE_AMP_DRV_LVL, 0x0f),
> > @@ -266,7 +266,7 @@ static const struct qmp_phy_init_tbl sm6115_ufsphy_pcs_tbl[] = {
> >   	QMP_PHY_INIT_CFG(QPHY_V2_PCS_RX_MIN_HIBERN8_TIME, 0x9a), /* 8 us */
> >   };
> > -static const struct qmp_phy_init_tbl sdm845_ufsphy_serdes_tbl[] = {
> > +static const struct qmp_phy_init_tbl sdm845_ufsphy_serdes[] = {
> >   	QMP_PHY_INIT_CFG(QSERDES_V3_COM_SYS_CLK_CTRL, 0x02),
> >   	QMP_PHY_INIT_CFG(QSERDES_V3_COM_BIAS_EN_CLKBUFLR_EN, 0x04),
> >   	QMP_PHY_INIT_CFG(QSERDES_V3_COM_BG_TIMER, 0x0a),
> > @@ -308,13 +308,13 @@ static const struct qmp_phy_init_tbl sdm845_ufsphy_serdes_tbl[] = {
> >   	QMP_PHY_INIT_CFG(QSERDES_V3_COM_VCO_TUNE_MAP, 0x44),
> >   };
> > -static const struct qmp_phy_init_tbl sdm845_ufsphy_tx_tbl[] = {
> > +static const struct qmp_phy_init_tbl sdm845_ufsphy_tx[] = {
> >   	QMP_PHY_INIT_CFG(QSERDES_V3_TX_LANE_MODE_1, 0x06),
> >   	QMP_PHY_INIT_CFG(QSERDES_V3_TX_RES_CODE_LANE_OFFSET_TX, 0x04),
> >   	QMP_PHY_INIT_CFG(QSERDES_V3_TX_RES_CODE_LANE_OFFSET_RX, 0x07),
> >   };
> > -static const struct qmp_phy_init_tbl sdm845_ufsphy_rx_tbl[] = {
> > +static const struct qmp_phy_init_tbl sdm845_ufsphy_rx[] = {
> >   	QMP_PHY_INIT_CFG(QSERDES_V3_RX_SIGDET_LVL, 0x24),
> >   	QMP_PHY_INIT_CFG(QSERDES_V3_RX_SIGDET_CNTRL, 0x0f),
> >   	QMP_PHY_INIT_CFG(QSERDES_V3_RX_SIGDET_DEGLITCH_CNTRL, 0x1e),
> > @@ -333,7 +333,7 @@ static const struct qmp_phy_init_tbl sdm845_ufsphy_rx_tbl[] = {
> >   	QMP_PHY_INIT_CFG(QSERDES_V3_RX_RX_MODE_00, 0x59),
> >   };
> > -static const struct qmp_phy_init_tbl sdm845_ufsphy_pcs_tbl[] = {
> > +static const struct qmp_phy_init_tbl sdm845_ufsphy_pcs[] = {
> >   	QMP_PHY_INIT_CFG(QPHY_V3_PCS_UFS_RX_SIGDET_CTRL2, 0x6e),
> >   	QMP_PHY_INIT_CFG(QPHY_V3_PCS_UFS_TX_LARGE_AMP_DRV_LVL, 0x0a),
> >   	QMP_PHY_INIT_CFG(QPHY_V3_PCS_UFS_TX_SMALL_AMP_DRV_LVL, 0x02),
> > @@ -344,7 +344,7 @@ static const struct qmp_phy_init_tbl sdm845_ufsphy_pcs_tbl[] = {
> >   	QMP_PHY_INIT_CFG(QPHY_V3_PCS_UFS_MULTI_LANE_CTRL1, 0x02),
> >   };
> > -static const struct qmp_phy_init_tbl sm8150_ufsphy_serdes_tbl[] = {
> > +static const struct qmp_phy_init_tbl sm8150_ufsphy_serdes[] = {
> >   	QMP_PHY_INIT_CFG(QSERDES_V4_COM_SYSCLK_EN_SEL, 0xd9),
> >   	QMP_PHY_INIT_CFG(QSERDES_V4_COM_HSCLK_SEL, 0x11),
> >   	QMP_PHY_INIT_CFG(QSERDES_V4_COM_HSCLK_HS_SWITCH_SEL, 0x00),
> > @@ -374,7 +374,7 @@ static const struct qmp_phy_init_tbl sm8150_ufsphy_serdes_tbl[] = {
> >   	QMP_PHY_INIT_CFG(QSERDES_V4_COM_VCO_TUNE_MAP, 0x06),
> >   };
> > -static const struct qmp_phy_init_tbl sm8150_ufsphy_tx_tbl[] = {
> > +static const struct qmp_phy_init_tbl sm8150_ufsphy_tx[] = {
> >   	QMP_PHY_INIT_CFG(QSERDES_V4_TX_PWM_GEAR_1_DIVIDER_BAND0_1, 0x06),
> >   	QMP_PHY_INIT_CFG(QSERDES_V4_TX_PWM_GEAR_2_DIVIDER_BAND0_1, 0x03),
> >   	QMP_PHY_INIT_CFG(QSERDES_V4_TX_PWM_GEAR_3_DIVIDER_BAND0_1, 0x01),
> > @@ -383,7 +383,7 @@ static const struct qmp_phy_init_tbl sm8150_ufsphy_tx_tbl[] = {
> >   	QMP_PHY_INIT_CFG(QSERDES_V4_TX_TRAN_DRVR_EMP_EN, 0x0c),
> >   };
> > -static const struct qmp_phy_init_tbl sm8150_ufsphy_rx_tbl[] = {
> > +static const struct qmp_phy_init_tbl sm8150_ufsphy_rx[] = {
> >   	QMP_PHY_INIT_CFG(QSERDES_V4_RX_SIGDET_LVL, 0x24),
> >   	QMP_PHY_INIT_CFG(QSERDES_V4_RX_SIGDET_CNTRL, 0x0f),
> >   	QMP_PHY_INIT_CFG(QSERDES_V4_RX_SIGDET_DEGLITCH_CNTRL, 0x1e),
> > @@ -421,7 +421,7 @@ static const struct qmp_phy_init_tbl sm8150_ufsphy_rx_tbl[] = {
> >   };
> > -static const struct qmp_phy_init_tbl sm8150_ufsphy_pcs_tbl[] = {
> > +static const struct qmp_phy_init_tbl sm8150_ufsphy_pcs[] = {
> >   	QMP_PHY_INIT_CFG(QPHY_V4_PCS_UFS_RX_SIGDET_CTRL2, 0x6d),
> >   	QMP_PHY_INIT_CFG(QPHY_V4_PCS_UFS_TX_LARGE_AMP_DRV_LVL, 0x0a),
> >   	QMP_PHY_INIT_CFG(QPHY_V4_PCS_UFS_TX_SMALL_AMP_DRV_LVL, 0x02),
> > @@ -431,7 +431,7 @@ static const struct qmp_phy_init_tbl sm8150_ufsphy_pcs_tbl[] = {
> >   	QMP_PHY_INIT_CFG(QPHY_V4_PCS_UFS_MULTI_LANE_CTRL1, 0x02),
> >   };
> > -static const struct qmp_phy_init_tbl sm8350_ufsphy_serdes_tbl[] = {
> > +static const struct qmp_phy_init_tbl sm8350_ufsphy_serdes[] = {
> >   	QMP_PHY_INIT_CFG(QSERDES_V5_COM_SYSCLK_EN_SEL, 0xd9),
> >   	QMP_PHY_INIT_CFG(QSERDES_V5_COM_HSCLK_SEL, 0x11),
> >   	QMP_PHY_INIT_CFG(QSERDES_V5_COM_HSCLK_HS_SWITCH_SEL, 0x00),
> > @@ -461,7 +461,7 @@ static const struct qmp_phy_init_tbl sm8350_ufsphy_serdes_tbl[] = {
> >   	QMP_PHY_INIT_CFG(QSERDES_V5_COM_VCO_TUNE_MAP, 0x06),
> >   };
> > -static const struct qmp_phy_init_tbl sm8350_ufsphy_tx_tbl[] = {
> > +static const struct qmp_phy_init_tbl sm8350_ufsphy_tx[] = {
> >   	QMP_PHY_INIT_CFG(QSERDES_V5_TX_PWM_GEAR_1_DIVIDER_BAND0_1, 0x06),
> >   	QMP_PHY_INIT_CFG(QSERDES_V5_TX_PWM_GEAR_2_DIVIDER_BAND0_1, 0x03),
> >   	QMP_PHY_INIT_CFG(QSERDES_V5_TX_PWM_GEAR_3_DIVIDER_BAND0_1, 0x01),
> > @@ -473,7 +473,7 @@ static const struct qmp_phy_init_tbl sm8350_ufsphy_tx_tbl[] = {
> >   	QMP_PHY_INIT_CFG(QSERDES_V5_TX_TRAN_DRVR_EMP_EN, 0x0c),
> >   };
> > -static const struct qmp_phy_init_tbl sm8350_ufsphy_rx_tbl[] = {
> > +static const struct qmp_phy_init_tbl sm8350_ufsphy_rx[] = {
> >   	QMP_PHY_INIT_CFG(QSERDES_V5_RX_SIGDET_LVL, 0x24),
> >   	QMP_PHY_INIT_CFG(QSERDES_V5_RX_SIGDET_CNTRL, 0x0f),
> >   	QMP_PHY_INIT_CFG(QSERDES_V5_RX_SIGDET_DEGLITCH_CNTRL, 0x1e),
> > @@ -513,7 +513,7 @@ static const struct qmp_phy_init_tbl sm8350_ufsphy_rx_tbl[] = {
> >   	QMP_PHY_INIT_CFG(QSERDES_V5_RX_DCC_CTRL1, 0x0c),
> >   };
> > -static const struct qmp_phy_init_tbl sm8350_ufsphy_pcs_tbl[] = {
> > +static const struct qmp_phy_init_tbl sm8350_ufsphy_pcs[] = {
> >   	QMP_PHY_INIT_CFG(QPHY_V5_PCS_UFS_RX_SIGDET_CTRL2, 0x6d),
> >   	QMP_PHY_INIT_CFG(QPHY_V5_PCS_UFS_TX_LARGE_AMP_DRV_LVL, 0x0a),
> >   	QMP_PHY_INIT_CFG(QPHY_V5_PCS_UFS_TX_SMALL_AMP_DRV_LVL, 0x02),
> > @@ -531,19 +531,24 @@ static const struct qmp_phy_init_tbl sm8350_ufsphy_pcs_tbl[] = {
> >   	QMP_PHY_INIT_CFG(QPHY_V5_PCS_UFS_MULTI_LANE_CTRL1, 0x02),
> >   };
> > +struct qmp_phy_cfg_tables {
> > +	/* Init sequence for PHY blocks - serdes, tx, rx, pcs */
> > +	const struct qmp_phy_init_tbl *serdes;
> > +	int serdes_num;
> > +	const struct qmp_phy_init_tbl *tx;
> > +	int tx_num;
> > +	const struct qmp_phy_init_tbl *rx;
> > +	int rx_num;
> > +	const struct qmp_phy_init_tbl *pcs;
> > +	int pcs_num;
> > +};
> > +
> >   /* struct qmp_phy_cfg - per-PHY initialization config */
> >   struct qmp_phy_cfg {
> >   	int lanes;
> > -	/* Init sequence for PHY blocks - serdes, tx, rx, pcs */
> > -	const struct qmp_phy_init_tbl *serdes_tbl;
> > -	int serdes_tbl_num;
> > -	const struct qmp_phy_init_tbl *tx_tbl;
> > -	int tx_tbl_num;
> > -	const struct qmp_phy_init_tbl *rx_tbl;
> > -	int rx_tbl_num;
> > -	const struct qmp_phy_init_tbl *pcs_tbl;
> > -	int pcs_tbl_num;
> > +	/* Main init sequence for PHY blocks - serdes, tx, rx, pcs */
> > +	const struct qmp_phy_cfg_tables tables;
> >   	/* clock ids to be requested */
> >   	const char * const *clk_list;
> > @@ -660,12 +665,14 @@ static const char * const qmp_phy_vreg_l[] = {
> >   static const struct qmp_phy_cfg msm8996_ufs_cfg = {
> >   	.lanes			= 1,
> > -	.serdes_tbl		= msm8996_ufs_serdes_tbl,
> > -	.serdes_tbl_num		= ARRAY_SIZE(msm8996_ufs_serdes_tbl),
> > -	.tx_tbl			= msm8996_ufs_tx_tbl,
> > -	.tx_tbl_num		= ARRAY_SIZE(msm8996_ufs_tx_tbl),
> > -	.rx_tbl			= msm8996_ufs_rx_tbl,
> > -	.rx_tbl_num		= ARRAY_SIZE(msm8996_ufs_rx_tbl),
> > +	.tables = {
> > +		.serdes		= msm8996_ufs_serdes,
> > +		.serdes_num	= ARRAY_SIZE(msm8996_ufs_serdes),
> > +		.tx		= msm8996_ufs_tx,
> > +		.tx_num		= ARRAY_SIZE(msm8996_ufs_tx),
> > +		.rx		= msm8996_ufs_rx,
> > +		.rx_num		= ARRAY_SIZE(msm8996_ufs_rx),
> > +	},
> >   	.clk_list		= msm8996_ufs_phy_clk_l,
> >   	.num_clks		= ARRAY_SIZE(msm8996_ufs_phy_clk_l),
> > @@ -685,14 +692,16 @@ static const struct qmp_phy_cfg msm8996_ufs_cfg = {
> >   static const struct qmp_phy_cfg sdm845_ufsphy_cfg = {
> >   	.lanes			= 2,
> > -	.serdes_tbl		= sdm845_ufsphy_serdes_tbl,
> > -	.serdes_tbl_num		= ARRAY_SIZE(sdm845_ufsphy_serdes_tbl),
> > -	.tx_tbl			= sdm845_ufsphy_tx_tbl,
> > -	.tx_tbl_num		= ARRAY_SIZE(sdm845_ufsphy_tx_tbl),
> > -	.rx_tbl			= sdm845_ufsphy_rx_tbl,
> > -	.rx_tbl_num		= ARRAY_SIZE(sdm845_ufsphy_rx_tbl),
> > -	.pcs_tbl		= sdm845_ufsphy_pcs_tbl,
> > -	.pcs_tbl_num		= ARRAY_SIZE(sdm845_ufsphy_pcs_tbl),
> > +	.tables = {
> > +		.serdes		= sdm845_ufsphy_serdes,
> > +		.serdes_num	= ARRAY_SIZE(sdm845_ufsphy_serdes),
> > +		.tx		= sdm845_ufsphy_tx,
> > +		.tx_num		= ARRAY_SIZE(sdm845_ufsphy_tx),
> > +		.rx		= sdm845_ufsphy_rx,
> > +		.rx_num		= ARRAY_SIZE(sdm845_ufsphy_rx),
> > +		.pcs		= sdm845_ufsphy_pcs,
> > +		.pcs_num	= ARRAY_SIZE(sdm845_ufsphy_pcs),
> > +	},
> >   	.clk_list		= sdm845_ufs_phy_clk_l,
> >   	.num_clks		= ARRAY_SIZE(sdm845_ufs_phy_clk_l),
> >   	.vreg_list		= qmp_phy_vreg_l,
> > @@ -709,14 +718,16 @@ static const struct qmp_phy_cfg sdm845_ufsphy_cfg = {
> >   static const struct qmp_phy_cfg sm6115_ufsphy_cfg = {
> >   	.lanes			= 1,
> > -	.serdes_tbl		= sm6115_ufsphy_serdes_tbl,
> > -	.serdes_tbl_num		= ARRAY_SIZE(sm6115_ufsphy_serdes_tbl),
> > -	.tx_tbl			= sm6115_ufsphy_tx_tbl,
> > -	.tx_tbl_num		= ARRAY_SIZE(sm6115_ufsphy_tx_tbl),
> > -	.rx_tbl			= sm6115_ufsphy_rx_tbl,
> > -	.rx_tbl_num		= ARRAY_SIZE(sm6115_ufsphy_rx_tbl),
> > -	.pcs_tbl		= sm6115_ufsphy_pcs_tbl,
> > -	.pcs_tbl_num		= ARRAY_SIZE(sm6115_ufsphy_pcs_tbl),
> > +	.tables = {
> > +		.serdes		= sm6115_ufsphy_serdes,
> > +		.serdes_num	= ARRAY_SIZE(sm6115_ufsphy_serdes),
> > +		.tx		= sm6115_ufsphy_tx,
> > +		.tx_num		= ARRAY_SIZE(sm6115_ufsphy_tx),
> > +		.rx		= sm6115_ufsphy_rx,
> > +		.rx_num		= ARRAY_SIZE(sm6115_ufsphy_rx),
> > +		.pcs		= sm6115_ufsphy_pcs,
> > +		.pcs_num	= ARRAY_SIZE(sm6115_ufsphy_pcs),
> > +	},
> >   	.clk_list		= sdm845_ufs_phy_clk_l,
> >   	.num_clks		= ARRAY_SIZE(sdm845_ufs_phy_clk_l),
> >   	.vreg_list		= qmp_phy_vreg_l,
> > @@ -732,14 +743,16 @@ static const struct qmp_phy_cfg sm6115_ufsphy_cfg = {
> >   static const struct qmp_phy_cfg sm8150_ufsphy_cfg = {
> >   	.lanes			= 2,
> > -	.serdes_tbl		= sm8150_ufsphy_serdes_tbl,
> > -	.serdes_tbl_num		= ARRAY_SIZE(sm8150_ufsphy_serdes_tbl),
> > -	.tx_tbl			= sm8150_ufsphy_tx_tbl,
> > -	.tx_tbl_num		= ARRAY_SIZE(sm8150_ufsphy_tx_tbl),
> > -	.rx_tbl			= sm8150_ufsphy_rx_tbl,
> > -	.rx_tbl_num		= ARRAY_SIZE(sm8150_ufsphy_rx_tbl),
> > -	.pcs_tbl		= sm8150_ufsphy_pcs_tbl,
> > -	.pcs_tbl_num		= ARRAY_SIZE(sm8150_ufsphy_pcs_tbl),
> > +	.tables = {
> > +		.serdes		= sm8150_ufsphy_serdes,
> > +		.serdes_num	= ARRAY_SIZE(sm8150_ufsphy_serdes),
> > +		.tx		= sm8150_ufsphy_tx,
> > +		.tx_num		= ARRAY_SIZE(sm8150_ufsphy_tx),
> > +		.rx		= sm8150_ufsphy_rx,
> > +		.rx_num		= ARRAY_SIZE(sm8150_ufsphy_rx),
> > +		.pcs		= sm8150_ufsphy_pcs,
> > +		.pcs_num	= ARRAY_SIZE(sm8150_ufsphy_pcs),
> > +	},
> >   	.clk_list		= sdm845_ufs_phy_clk_l,
> >   	.num_clks		= ARRAY_SIZE(sdm845_ufs_phy_clk_l),
> >   	.vreg_list		= qmp_phy_vreg_l,
> > @@ -754,14 +767,16 @@ static const struct qmp_phy_cfg sm8150_ufsphy_cfg = {
> >   static const struct qmp_phy_cfg sm8350_ufsphy_cfg = {
> >   	.lanes			= 2,
> > -	.serdes_tbl		= sm8350_ufsphy_serdes_tbl,
> > -	.serdes_tbl_num		= ARRAY_SIZE(sm8350_ufsphy_serdes_tbl),
> > -	.tx_tbl			= sm8350_ufsphy_tx_tbl,
> > -	.tx_tbl_num		= ARRAY_SIZE(sm8350_ufsphy_tx_tbl),
> > -	.rx_tbl			= sm8350_ufsphy_rx_tbl,
> > -	.rx_tbl_num		= ARRAY_SIZE(sm8350_ufsphy_rx_tbl),
> > -	.pcs_tbl		= sm8350_ufsphy_pcs_tbl,
> > -	.pcs_tbl_num		= ARRAY_SIZE(sm8350_ufsphy_pcs_tbl),
> > +	.tables = {
> > +		.serdes		= sm8350_ufsphy_serdes,
> > +		.serdes_num	= ARRAY_SIZE(sm8350_ufsphy_serdes),
> > +		.tx		= sm8350_ufsphy_tx,
> > +		.tx_num		= ARRAY_SIZE(sm8350_ufsphy_tx),
> > +		.rx		= sm8350_ufsphy_rx,
> > +		.rx_num		= ARRAY_SIZE(sm8350_ufsphy_rx),
> > +		.pcs		= sm8350_ufsphy_pcs,
> > +		.pcs_num	= ARRAY_SIZE(sm8350_ufsphy_pcs),
> > +	},
> >   	.clk_list		= sdm845_ufs_phy_clk_l,
> >   	.num_clks		= ARRAY_SIZE(sdm845_ufs_phy_clk_l),
> >   	.vreg_list		= qmp_phy_vreg_l,
> > @@ -776,14 +791,16 @@ static const struct qmp_phy_cfg sm8350_ufsphy_cfg = {
> >   static const struct qmp_phy_cfg sm8450_ufsphy_cfg = {
> >   	.lanes			= 2,
> > -	.serdes_tbl		= sm8350_ufsphy_serdes_tbl,
> > -	.serdes_tbl_num		= ARRAY_SIZE(sm8350_ufsphy_serdes_tbl),
> > -	.tx_tbl			= sm8350_ufsphy_tx_tbl,
> > -	.tx_tbl_num		= ARRAY_SIZE(sm8350_ufsphy_tx_tbl),
> > -	.rx_tbl			= sm8350_ufsphy_rx_tbl,
> > -	.rx_tbl_num		= ARRAY_SIZE(sm8350_ufsphy_rx_tbl),
> > -	.pcs_tbl		= sm8350_ufsphy_pcs_tbl,
> > -	.pcs_tbl_num		= ARRAY_SIZE(sm8350_ufsphy_pcs_tbl),
> > +	.tables = {
> > +		.serdes		= sm8350_ufsphy_serdes,
> > +		.serdes_num	= ARRAY_SIZE(sm8350_ufsphy_serdes),
> > +		.tx		= sm8350_ufsphy_tx,
> > +		.tx_num		= ARRAY_SIZE(sm8350_ufsphy_tx),
> > +		.rx		= sm8350_ufsphy_rx,
> > +		.rx_num		= ARRAY_SIZE(sm8350_ufsphy_rx),
> > +		.pcs		= sm8350_ufsphy_pcs,
> > +		.pcs_num	= ARRAY_SIZE(sm8350_ufsphy_pcs),
> > +	},
> >   	.clk_list		= sm8450_ufs_phy_clk_l,
> >   	.num_clks		= ARRAY_SIZE(sm8450_ufs_phy_clk_l),
> >   	.vreg_list		= qmp_phy_vreg_l,
> > @@ -826,16 +843,43 @@ static void qmp_ufs_configure(void __iomem *base,
> >   	qmp_ufs_configure_lane(base, regs, tbl, num, 0xff);
> >   }
> > -static int qmp_ufs_serdes_init(struct qmp_phy *qphy)
> > +static void qmp_ufs_serdes_init(struct qmp_phy *qphy, const struct qmp_phy_cfg_tables *tables)
> >   {
> >   	const struct qmp_phy_cfg *cfg = qphy->cfg;
> >   	void __iomem *serdes = qphy->serdes;
> > -	const struct qmp_phy_init_tbl *serdes_tbl = cfg->serdes_tbl;
> > -	int serdes_tbl_num = cfg->serdes_tbl_num;
> > -	qmp_ufs_configure(serdes, cfg->regs, serdes_tbl, serdes_tbl_num);
> > +	if (!tables)
> > +		return;
> > -	return 0;
> > +	qmp_ufs_configure(serdes, cfg->regs, tables->serdes, tables->serdes_num);
> > +}
> > +
> > +static void qmp_ufs_lanes_init(struct qmp_phy *qphy, const struct qmp_phy_cfg_tables *tables)
> > +{
> > +	const struct qmp_phy_cfg *cfg = qphy->cfg;
> > +	void __iomem *tx = qphy->tx;
> > +	void __iomem *rx = qphy->rx;
> > +
> > +	qmp_ufs_configure_lane(tx, cfg->regs, tables->tx, tables->tx_num, 1);
> > +
> > +	if (cfg->lanes >= 2)
> > +		qmp_ufs_configure_lane(qphy->tx2, cfg->regs, tables->tx, tables->tx_num, 2);
> > +
> > +	qmp_ufs_configure_lane(rx, cfg->regs, tables->rx, tables->rx_num, 1);
> > +
> > +	if (cfg->lanes >= 2)
> > +		qmp_ufs_configure_lane(qphy->rx2, cfg->regs, tables->rx, tables->rx_num, 2);
> > +}
> > +
> > +static void qmp_ufs_pcs_init(struct qmp_phy *qphy, const struct qmp_phy_cfg_tables *tables)
> > +{
> > +	const struct qmp_phy_cfg *cfg = qphy->cfg;
> > +	void __iomem *pcs = qphy->pcs;
> > +
> > +	if (!tables)
> > +		return;
> > +
> > +	qmp_ufs_configure(pcs, cfg->regs, tables->pcs, tables->pcs_num);
> >   }
> >   static int qmp_ufs_com_init(struct qmp_phy *qphy)
> > @@ -933,31 +977,16 @@ static int qmp_ufs_power_on(struct phy *phy)
> >   	struct qmp_phy *qphy = phy_get_drvdata(phy);
> >   	struct qcom_qmp *qmp = qphy->qmp;
> >   	const struct qmp_phy_cfg *cfg = qphy->cfg;
> > -	void __iomem *tx = qphy->tx;
> > -	void __iomem *rx = qphy->rx;
> >   	void __iomem *pcs = qphy->pcs;
> >   	void __iomem *status;
> >   	unsigned int mask, val, ready;
> >   	int ret;
> > -	qmp_ufs_serdes_init(qphy);
> > -
> > -	/* Tx, Rx, and PCS configurations */
> > -	qmp_ufs_configure_lane(tx, cfg->regs, cfg->tx_tbl, cfg->tx_tbl_num, 1);
> > +	qmp_ufs_serdes_init(qphy, &cfg->tables);
> > -	if (cfg->lanes >= 2) {
> > -		qmp_ufs_configure_lane(qphy->tx2, cfg->regs,
> > -					cfg->tx_tbl, cfg->tx_tbl_num, 2);
> > -	}
> > -
> > -	qmp_ufs_configure_lane(rx, cfg->regs, cfg->rx_tbl, cfg->rx_tbl_num, 1);
> > -
> > -	if (cfg->lanes >= 2) {
> > -		qmp_ufs_configure_lane(qphy->rx2, cfg->regs,
> > -					cfg->rx_tbl, cfg->rx_tbl_num, 2);
> > -	}
> > +	qmp_ufs_lanes_init(qphy, &cfg->tables);
> > -	qmp_ufs_configure(pcs, cfg->regs, cfg->pcs_tbl, cfg->pcs_tbl_num);
> > +	qmp_ufs_pcs_init(qphy, &cfg->tables);
> 
> I'd suggest going straight to qmp_ufs_init_registers, which would contain
> both serdes, lanes and pcs inits.
> 

That adds one more level of indirection which may not be needed here. Moreover,
I'm trying to be in sync with other qmp drivers, specifically the pcie one.
This helps in working with these drivers.

Thanks,
Mani

> >   	ret = reset_control_deassert(qmp->ufs_reset);
> >   	if (ret)
> 
> -- 
> With best wishes
> Dmitry
>
Dmitry Baryshkov Oct. 31, 2022, 6:52 p.m. UTC | #11
On 31/10/2022 17:56, Manivannan Sadhasivam wrote:
> On Sun, Oct 30, 2022 at 12:48:21AM +0300, Dmitry Baryshkov wrote:
>> On 29/10/2022 17:16, Manivannan Sadhasivam wrote:
>>> Starting from UFS controller v4, Qcom supports dual gear mode (i.e., the
>>> controller/PHY can be configured to run in two gear speeds). But that
>>> requires an agreement between the UFS controller and the UFS device.
>>> This commit finds the max gear supported by both controller and device
>>> then decides which one to use.
>>>
>>> UFS controller's max gear can be read from the REG_UFS_PARAM0 register and
>>> UFS device's max gear can be read from the "max-gear" devicetree property.
>>>
>>> The UFS PHY also needs to be configured with the decided gear using the
>>> phy_set_mode_ext() API.
>>>
>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>> ---
>>>    drivers/ufs/host/ufs-qcom.c | 35 ++++++++++++++++++++++++++++++++---
>>>    drivers/ufs/host/ufs-qcom.h |  4 ++++
>>>    2 files changed, 36 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>>> index f952cc76919f..268463e92d67 100644
>>> --- a/drivers/ufs/host/ufs-qcom.c
>>> +++ b/drivers/ufs/host/ufs-qcom.c
>>> @@ -281,6 +281,9 @@ static int ufs_qcom_host_reset(struct ufs_hba *hba)
>>>    static u32 ufs_qcom_get_hs_gear(struct ufs_hba *hba, u32 hs_gear)
>>>    {
>>>    	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>>> +	struct device *dev = hba->dev;
>>> +	u32 max_gear, hcd_max_gear, reg;
>>> +	int ret;
>>>    	if (host->hw_ver.major == 0x1) {
>>>    		/*
>>> @@ -292,8 +295,33 @@ static u32 ufs_qcom_get_hs_gear(struct ufs_hba *hba, u32 hs_gear)
>>>    		 */
>>>    		if (hs_gear > UFS_HS_G2)
>>>    			return UFS_HS_G2;
>>> +	} else if (host->hw_ver.major > 0x3) {
>>> +		/*
>>> +		 * Starting from UFS controller v4, Qcom supports dual gear mode (i.e., the
>>> +		 * controller/PHY can be configured to run in two gear speeds). But that
>>> +		 * requires an agreement between the UFS controller and the device. Below
>>> +		 * code tries to find the max gear of both and decides which gear to use.
>>> +		 *
>>> +		 * First get the max gear supported by the UFS device if available.
>>> +		 * If the property is not defined in devicetree, then use the default gear.
>>> +		 */
>>> +		ret = of_property_read_u32(dev->of_node, "max-gear", &max_gear);
>>> +		if (ret)
>>> +			goto err_out;
>>
>> Can we detect the UFS device's max gear somehow? If not, the 'max-gear'
>> property name doesn't sound good. Maybe calling it 'device-gear' would be
>> better.
>>
> 
> UFS device probing depends on PHY init sequence. So technically we cannot know
> the max gear of the device without using an init sequence, but this information
> is static and should be known to a board manufacturer. That's why I decided to
> use this property. Another option is to use a fixed init sequence for probing
> the device and do a re-init after knowing it's max gear but that is not
> recommended.
> 
> We need "max" keyword because this property specifies the maximum gear at which
> the device could operate and not necessarily the gear at which it operates.
> Maybe, "max-device-gear" would make it clear.

Ack, thank you for the explanation. Yes, from my opinion max-device-gear 
is better. Let's see what Rob and Krzysztof would say.
Manivannan Sadhasivam Nov. 1, 2022, 2:41 p.m. UTC | #12
On Mon, Oct 31, 2022 at 09:50:59PM +0300, Dmitry Baryshkov wrote:
> On 31/10/2022 18:46, Manivannan Sadhasivam wrote:
> > On Sun, Oct 30, 2022 at 12:50:50AM +0300, Dmitry Baryshkov wrote:
> > > On 29/10/2022 17:16, Manivannan Sadhasivam wrote:
> > > > As done for Qcom PCIe PHY driver, let's move the register settings to the
> > > > common qmp_phy_cfg_tables struct. This helps in adding any additional PHY
> > > > settings needed for functionalities like HS-G4 in the future by adding one
> > > > more instance of the qmp_phy_cfg_tables.
> > > > 
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > ---
> > > >    drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 223 +++++++++++++-----------
> > > >    1 file changed, 126 insertions(+), 97 deletions(-)
> > > > 
> > > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c

[...]

> > > >    static int qmp_ufs_com_init(struct qmp_phy *qphy)
> > > > @@ -933,31 +977,16 @@ static int qmp_ufs_power_on(struct phy *phy)
> > > >    	struct qmp_phy *qphy = phy_get_drvdata(phy);
> > > >    	struct qcom_qmp *qmp = qphy->qmp;
> > > >    	const struct qmp_phy_cfg *cfg = qphy->cfg;
> > > > -	void __iomem *tx = qphy->tx;
> > > > -	void __iomem *rx = qphy->rx;
> > > >    	void __iomem *pcs = qphy->pcs;
> > > >    	void __iomem *status;
> > > >    	unsigned int mask, val, ready;
> > > >    	int ret;
> > > > -	qmp_ufs_serdes_init(qphy);
> > > > -
> > > > -	/* Tx, Rx, and PCS configurations */
> > > > -	qmp_ufs_configure_lane(tx, cfg->regs, cfg->tx_tbl, cfg->tx_tbl_num, 1);
> > > > +	qmp_ufs_serdes_init(qphy, &cfg->tables);
> > > > -	if (cfg->lanes >= 2) {
> > > > -		qmp_ufs_configure_lane(qphy->tx2, cfg->regs,
> > > > -					cfg->tx_tbl, cfg->tx_tbl_num, 2);
> > > > -	}
> > > > -
> > > > -	qmp_ufs_configure_lane(rx, cfg->regs, cfg->rx_tbl, cfg->rx_tbl_num, 1);
> > > > -
> > > > -	if (cfg->lanes >= 2) {
> > > > -		qmp_ufs_configure_lane(qphy->rx2, cfg->regs,
> > > > -					cfg->rx_tbl, cfg->rx_tbl_num, 2);
> > > > -	}
> > > > +	qmp_ufs_lanes_init(qphy, &cfg->tables);
> > > > -	qmp_ufs_configure(pcs, cfg->regs, cfg->pcs_tbl, cfg->pcs_tbl_num);
> > > > +	qmp_ufs_pcs_init(qphy, &cfg->tables);
> > > 
> > > I'd suggest going straight to qmp_ufs_init_registers, which would contain
> > > both serdes, lanes and pcs inits.
> > > 
> > 
> > That adds one more level of indirection which may not be needed here. Moreover,
> > I'm trying to be in sync with other qmp drivers, specifically the pcie one.
> > This helps in working with these drivers.
> 
> Yes, I understand. However I hope that the respective patchset (including
> [1]) will be merged soon. Thus I suggest skipping the step and using the
> same function already.
> 
> [1] https://lore.kernel.org/linux-phy/20221028133603.18470-10-johan+linaro@kernel.org/
> 

Ah, I missed this series. Will use the common function then.

Thanks,
Mani

> > 
> > Thanks,
> > Mani
> > 
> > > >    	ret = reset_control_deassert(qmp->ufs_reset);
> > > >    	if (ret)
> > > 
> > > -- 
> > > With best wishes
> > > Dmitry
> > > 
> > 
> 
> -- 
> With best wishes
> Dmitry
>