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

Larisa Ileana Grigore <[email protected]>
Newsgroups org.kernel.vger.linux-scsi,dev.linux.lists.imx,org.infradead.lists.linux-arm-kernel,org.kernel.vger.linux-arm-msm,org.kernel.vger.linux-devicetree,org.kernel.vger.linux-kernel
Message-ID <[email protected]> 
On 8/26/2026 6:20 PM, Frank Li wrote:
> 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
> 
Thank you! I’ll address it in the next revision.

>> 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 <[email protected]>
>> ---
> 
> Reviewed-by: Frank Li <[email protected]>
> 
>>   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 <[email protected]>");
>>   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
>>
>>
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.