Re: [PATCH 04/10] scsi: ufs: Move Versal2 M-PHY CREG access helpers into ufshcd-dwc

[Frank Li <Frank.li@oss.nxp.com>]

On Wed, Aug 26, 2026 at 03:40:42PM +0200, Larisa Grigore wrote:
>
> The AMD Versal Gen 2 UFS platform driver carries local helpers to access
> M-PHY control registers through the UniPro CREG interface.
>
> Move these helpers into the common Synopsys DWC layer as
> ufshcd_dwc_phy_reg_read() and ufshcd_dwc_phy_reg_write(). This avoids code
> duplication in DWC-based glue drivers and provides a single implementation
> for future reuse.

Nit: add empty line between paragraph

> Dropped `static` from `phy_write_attrs` and `phy_read_attrs` since there
> is no benefit in keeping them persistent.
>
> Update the AMD Versal2 glue driver to use the new ufshcd-dwc helpers.
>
> Signed-off-by: Larisa Grigore <larisa.grigore@oss.nxp.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/ufs/host/ufs-amd-versal2.c | 85 ++++++------------------------
>  drivers/ufs/host/ufshcd-dwc.c      | 77 +++++++++++++++++++++++++++
>  drivers/ufs/host/ufshcd-dwc.h      |  2 +
>  3 files changed, 96 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/ufs/host/ufs-amd-versal2.c b/drivers/ufs/host/ufs-amd-versal2.c
> index 0d4d5e98ad65..c7da95999c85 100644
> --- a/drivers/ufs/host/ufs-amd-versal2.c
> +++ b/drivers/ufs/host/ufs-amd-versal2.c
> @@ -43,57 +43,6 @@ struct ufs_versal2_host {
>         u8 ctlecompval1;
>  };
>
> -static int ufs_versal2_phy_reg_write(struct ufs_hba *hba, u32 addr, u32 val)
> -{
> -       static struct ufshcd_dme_attr_val phy_write_attrs[] = {
> -               { UIC_ARG_MIB(CBCREGADDRLSB), 0, DME_LOCAL },
> -               { UIC_ARG_MIB(CBCREGADDRMSB), 0, DME_LOCAL },
> -               { UIC_ARG_MIB(CBCREGWRLSB), 0, DME_LOCAL },
> -               { UIC_ARG_MIB(CBCREGWRMSB), 0, DME_LOCAL },
> -               { UIC_ARG_MIB(CBCREGRDWRSEL), 1, DME_LOCAL },
> -               { UIC_ARG_MIB(VS_MPHYCFGUPDT), 1, DME_LOCAL }
> -       };
> -
> -       phy_write_attrs[0].mib_val = (u8)addr;
> -       phy_write_attrs[1].mib_val = (u8)(addr >> 8);
> -       phy_write_attrs[2].mib_val = (u8)val;
> -       phy_write_attrs[3].mib_val = (u8)(val >> 8);
> -
> -       return ufshcd_dwc_dme_set_attrs(hba, phy_write_attrs, ARRAY_SIZE(phy_write_attrs));
> -}
> -
> -static int ufs_versal2_phy_reg_read(struct ufs_hba *hba, u32 addr, u32 *val)
> -{
> -       u32 mib_val;
> -       int ret;
> -       static struct ufshcd_dme_attr_val phy_read_attrs[] = {
> -               { UIC_ARG_MIB(CBCREGADDRLSB), 0, DME_LOCAL },
> -               { UIC_ARG_MIB(CBCREGADDRMSB), 0, DME_LOCAL },
> -               { UIC_ARG_MIB(CBCREGRDWRSEL), 0, DME_LOCAL },
> -               { UIC_ARG_MIB(VS_MPHYCFGUPDT), 1, DME_LOCAL }
> -       };
> -
> -       phy_read_attrs[0].mib_val = (u8)addr;
> -       phy_read_attrs[1].mib_val = (u8)(addr >> 8);
> -
> -       ret = ufshcd_dwc_dme_set_attrs(hba, phy_read_attrs, ARRAY_SIZE(phy_read_attrs));
> -       if (ret)
> -               return ret;
> -
> -       ret = ufshcd_dme_get(hba, UIC_ARG_MIB(CBCREGRDLSB), &mib_val);
> -       if (ret)
> -               return ret;
> -
> -       *val = mib_val;
> -       ret = ufshcd_dme_get(hba, UIC_ARG_MIB(CBCREGRDMSB), &mib_val);
> -       if (ret)
> -               return ret;
> -
> -       *val |= (mib_val << 8);
> -
> -       return 0;
> -}
> -
>  static int ufs_versal2_enable_phy(struct ufs_hba *hba)
>  {
>         u32 offset, reg;
> @@ -162,64 +111,64 @@ static int ufs_versal2_setup_phy(struct ufs_hba *hba)
>         u32 reg;
>
>         /* Bypass RX-AFE offset calibrations (ATT/CTLE) */
> -       ret = ufs_versal2_phy_reg_read(hba, FAST_FLAGS(0), &reg);
> +       ret = ufshcd_dwc_phy_reg_read(hba, FAST_FLAGS(0), &reg);
>         if (ret)
>                 return ret;
>
>         reg |= MPHY_FAST_RX_AFE_CAL;
> -       ret = ufs_versal2_phy_reg_write(hba, FAST_FLAGS(0), reg);
> +       ret = ufshcd_dwc_phy_reg_write(hba, FAST_FLAGS(0), reg);
>         if (ret)
>                 return ret;
>
> -       ret = ufs_versal2_phy_reg_read(hba, FAST_FLAGS(1), &reg);
> +       ret = ufshcd_dwc_phy_reg_read(hba, FAST_FLAGS(1), &reg);
>         if (ret)
>                 return ret;
>
>         reg |= MPHY_FAST_RX_AFE_CAL;
> -       ret = ufs_versal2_phy_reg_write(hba, FAST_FLAGS(1), reg);
> +       ret = ufshcd_dwc_phy_reg_write(hba, FAST_FLAGS(1), reg);
>         if (ret)
>                 return ret;
>
>         /* Program ATT and CTLE compensation values */
>         if (host->attcompval0) {
> -               ret = ufs_versal2_phy_reg_write(hba, RX_AFE_ATT_IDAC(0), host->attcompval0);
> +               ret = ufshcd_dwc_phy_reg_write(hba, RX_AFE_ATT_IDAC(0), host->attcompval0);
>                 if (ret)
>                         return ret;
>         }
>
>         if (host->attcompval1) {
> -               ret = ufs_versal2_phy_reg_write(hba, RX_AFE_ATT_IDAC(1), host->attcompval1);
> +               ret = ufshcd_dwc_phy_reg_write(hba, RX_AFE_ATT_IDAC(1), host->attcompval1);
>                 if (ret)
>                         return ret;
>         }
>
>         if (host->ctlecompval0) {
> -               ret = ufs_versal2_phy_reg_write(hba, RX_AFE_CTLE_IDAC(0), host->ctlecompval0);
> +               ret = ufshcd_dwc_phy_reg_write(hba, RX_AFE_CTLE_IDAC(0), host->ctlecompval0);
>                 if (ret)
>                         return ret;
>         }
>
>         if (host->ctlecompval1) {
> -               ret = ufs_versal2_phy_reg_write(hba, RX_AFE_CTLE_IDAC(1), host->ctlecompval1);
> +               ret = ufshcd_dwc_phy_reg_write(hba, RX_AFE_CTLE_IDAC(1), host->ctlecompval1);
>                 if (ret)
>                         return ret;
>         }
>
> -       ret = ufs_versal2_phy_reg_read(hba, FW_CALIB_CCFG(0), &reg);
> +       ret = ufshcd_dwc_phy_reg_read(hba, FW_CALIB_CCFG(0), &reg);
>         if (ret)
>                 return ret;
>
>         reg |= MPHY_FW_CALIB_CFG_VAL;
> -       ret = ufs_versal2_phy_reg_write(hba, FW_CALIB_CCFG(0), reg);
> +       ret = ufshcd_dwc_phy_reg_write(hba, FW_CALIB_CCFG(0), reg);
>         if (ret)
>                 return ret;
>
> -       ret = ufs_versal2_phy_reg_read(hba, FW_CALIB_CCFG(1), &reg);
> +       ret = ufshcd_dwc_phy_reg_read(hba, FW_CALIB_CCFG(1), &reg);
>         if (ret)
>                 return ret;
>
>         reg |= MPHY_FW_CALIB_CFG_VAL;
> -       return ufs_versal2_phy_reg_write(hba, FW_CALIB_CCFG(1), reg);
> +       return ufshcd_dwc_phy_reg_write(hba, FW_CALIB_CCFG(1), reg);
>  }
>
>  static int ufs_versal2_phy_init(struct ufs_hba *hba)
> @@ -406,7 +355,7 @@ static int ufs_versal2_phy_ratesel(struct ufs_hba *hba, u32 activelanes, u32 rx_
>
>         for (lane = 0; lane < activelanes; lane++) {
>                 time_left = TIMEOUT_MICROSEC;
> -               ret = ufs_versal2_phy_reg_read(hba, RX_OVRD_IN_1(lane), &reg);
> +               ret = ufshcd_dwc_phy_reg_read(hba, RX_OVRD_IN_1(lane), &reg);
>                 if (ret)
>                         return ret;
>
> @@ -416,12 +365,12 @@ static int ufs_versal2_phy_ratesel(struct ufs_hba *hba, u32 activelanes, u32 rx_
>                 else
>                         reg &= ~MPHY_RX_OVRD_VAL;
>
> -               ret = ufs_versal2_phy_reg_write(hba, RX_OVRD_IN_1(lane), reg);
> +               ret = ufshcd_dwc_phy_reg_write(hba, RX_OVRD_IN_1(lane), reg);
>                 if (ret)
>                         return ret;
>
>                 do {
> -                       ret = ufs_versal2_phy_reg_read(hba, RX_PCS_OUT(lane), &reg);
> +                       ret = ufshcd_dwc_phy_reg_read(hba, RX_PCS_OUT(lane), &reg);
>                         if (ret)
>                                 return ret;
>
> @@ -483,12 +432,12 @@ static int ufs_versal2_pwr_change_notify(struct ufs_hba *hba, enum ufs_notify_ch
>
>                 /* Remove rx_req override */
>                 for (lane = 0; lane < dev_req_params->lane_tx; lane++) {
> -                       ret = ufs_versal2_phy_reg_read(hba, RX_OVRD_IN_1(lane), &reg);
> +                       ret = ufshcd_dwc_phy_reg_read(hba, RX_OVRD_IN_1(lane), &reg);
>                         if (ret)
>                                 return ret;
>
>                         reg &= ~MPHY_RX_OVRD_EN;
> -                       ret = ufs_versal2_phy_reg_write(hba, RX_OVRD_IN_1(lane), reg);
> +                       ret = ufshcd_dwc_phy_reg_write(hba, RX_OVRD_IN_1(lane), reg);
>                         if (ret)
>                                 return ret;
>                 }
> diff --git a/drivers/ufs/host/ufshcd-dwc.c b/drivers/ufs/host/ufshcd-dwc.c
> index 21b1cf912dcc..45920793d1a8 100644
> --- a/drivers/ufs/host/ufshcd-dwc.c
> +++ b/drivers/ufs/host/ufshcd-dwc.c
> @@ -141,6 +141,83 @@ int ufshcd_dwc_link_startup_notify(struct ufs_hba *hba,
>  }
>  EXPORT_SYMBOL(ufshcd_dwc_link_startup_notify);
>
> +/**
> + * ufshcd_dwc_phy_reg_write - Write a DWC M-PHY CREG register
> + * @hba: private structure pointer
> + * @addr: M-PHY CREG register address
> + * @val: value to write
> + *
> + * Write a 16-bit M-PHY CREG register through the Synopsys DesignWare
> + * UniPro indirect register access interface.
> + *
> + * Return: 0 on success, non-zero value on failure.
> + */
> +int ufshcd_dwc_phy_reg_write(struct ufs_hba *hba, u32 addr, u32 val)
> +{
> +       struct ufshcd_dme_attr_val phy_write_attrs[] = {
> +               { UIC_ARG_MIB(CBCREGADDRLSB), 0, DME_LOCAL },
> +               { UIC_ARG_MIB(CBCREGADDRMSB), 0, DME_LOCAL },
> +               { UIC_ARG_MIB(CBCREGWRLSB), 0, DME_LOCAL },
> +               { UIC_ARG_MIB(CBCREGWRMSB), 0, DME_LOCAL },
> +               { UIC_ARG_MIB(CBCREGRDWRSEL), 1, DME_LOCAL },
> +               { UIC_ARG_MIB(VS_MPHYCFGUPDT), 1, DME_LOCAL }
> +       };
> +
> +       phy_write_attrs[0].mib_val = (u8)addr;
> +       phy_write_attrs[1].mib_val = (u8)(addr >> 8);
> +       phy_write_attrs[2].mib_val = (u8)val;
> +       phy_write_attrs[3].mib_val = (u8)(val >> 8);
> +
> +       return ufshcd_dwc_dme_set_attrs(hba, phy_write_attrs,
> +                                       ARRAY_SIZE(phy_write_attrs));
> +}
> +EXPORT_SYMBOL(ufshcd_dwc_phy_reg_write);
> +
> +/**
> + * ufshcd_dwc_phy_reg_read - Read a DWC M-PHY CREG register
> + * @hba: private structure pointer
> + * @addr: M-PHY CREG register address
> + * @val: pointer where the read value is stored
> + *
> + * Read a 16-bit M-PHY CREG register through the Synopsys DesignWare
> + * UniPro indirect register access interface.
> + *
> + * Return: 0 on success, non-zero value on failure.
> + */
> +int ufshcd_dwc_phy_reg_read(struct ufs_hba *hba, u32 addr, u32 *val)
> +{
> +       struct ufshcd_dme_attr_val phy_read_attrs[] = {
> +               { UIC_ARG_MIB(CBCREGADDRLSB), 0, DME_LOCAL },
> +               { UIC_ARG_MIB(CBCREGADDRMSB), 0, DME_LOCAL },
> +               { UIC_ARG_MIB(CBCREGRDWRSEL), 0, DME_LOCAL },
> +               { UIC_ARG_MIB(VS_MPHYCFGUPDT), 1, DME_LOCAL }
> +       };
> +       u32 mib_val;
> +       int ret;
> +
> +       phy_read_attrs[0].mib_val = (u8)addr;
> +       phy_read_attrs[1].mib_val = (u8)(addr >> 8);
> +
> +       ret = ufshcd_dwc_dme_set_attrs(hba, phy_read_attrs,
> +                                      ARRAY_SIZE(phy_read_attrs));
> +       if (ret)
> +               return ret;
> +
> +       ret = ufshcd_dme_get(hba, UIC_ARG_MIB(CBCREGRDLSB), &mib_val);
> +       if (ret)
> +               return ret;
> +
> +       *val = mib_val;
> +       ret = ufshcd_dme_get(hba, UIC_ARG_MIB(CBCREGRDMSB), &mib_val);
> +       if (ret)
> +               return ret;
> +
> +       *val |= (mib_val << 8);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(ufshcd_dwc_phy_reg_read);
> +
>  MODULE_AUTHOR("Joao Pinto <Joao.Pinto@synopsys.com>");
>  MODULE_DESCRIPTION("UFS Host driver for Synopsys Designware Core");
>  MODULE_LICENSE("Dual BSD/GPL");
> diff --git a/drivers/ufs/host/ufshcd-dwc.h b/drivers/ufs/host/ufshcd-dwc.h
> index 0406f2bb576b..d8673cd54d4c 100644
> --- a/drivers/ufs/host/ufshcd-dwc.h
> +++ b/drivers/ufs/host/ufshcd-dwc.h
> @@ -45,4 +45,6 @@ int ufshcd_dwc_link_startup_notify(struct ufs_hba *hba,
>                                         enum ufs_notify_change_status status);
>  int ufshcd_dwc_dme_set_attrs(struct ufs_hba *hba,
>                                 const struct ufshcd_dme_attr_val *v, int n);
> +int ufshcd_dwc_phy_reg_write(struct ufs_hba *hba, u32 addr, u32 val);
> +int ufshcd_dwc_phy_reg_read(struct ufs_hba *hba, u32 addr, u32 *val);
>  #endif /* End of Header */
> --
> 2.43.0
>
>