Re: [PATCH 1/3] ufs: add support for DesignWare Controller

Hi,

On 19/09/2024 11:17, Venkatesh Yadav Abbarapu wrote:

This patch has the goal to add support for DesignWare UFS Controller specific operations.

This is based on linux kernel commit: "drivers/scsi/ufs/ufshcd-dwc.c: ufs: add support for DesignWare Controller" (sha1: 4b9ffb5a353bdee49f1f477ffe2b95ab3f9cbc0c) It is ported from linux kernel 6.11-rc1.

Signed-off-by: Venkatesh Yadav Abbarapu venkatesh.abbarapu@amd.com

drivers/ufs/ufs.h | 11 +++ drivers/ufs/ufshcd-dwc.c | 141 +++++++++++++++++++++++++++++++++++++++ drivers/ufs/ufshcd-dwc.h | 23 +++++++ drivers/ufs/ufshci-dwc.h | 32 +++++++++ 4 files changed, 207 insertions(+) create mode 100644 drivers/ufs/ufshcd-dwc.c create mode 100644 drivers/ufs/ufshcd-dwc.h create mode 100644 drivers/ufs/ufshci-dwc.h

diff --git a/drivers/ufs/ufs.h b/drivers/ufs/ufs.h index e8a1441156..83837d5817 100644 --- a/drivers/ufs/ufs.h +++ b/drivers/ufs/ufs.h @@ -850,6 +850,7 @@ struct ufs_hba { struct ufs_pwr_mode_info max_pwr_info;

struct ufs_dev_cmd dev_cmd;

• enum uic_link_state uic_link_state;

Will this be really used ?

};

static inline int ufshcd_ops_init(struct ufs_hba *hba) @@ -878,6 +879,14 @@ static inline int ufshcd_ops_link_startup_notify(struct ufs_hba *hba, return 0; }

+static inline int ufshcd_vops_phy_initialization(struct ufs_hba *hba) +{

• if (hba->ops && hba->ops->phy_initialization)

• return hba->ops->phy_initialization(hba);
  

• return 0;

+}

There's no users of hba->ops->phy_initialization, and the patch 2 doesn't even add an user, so I think you can also drop this....

• /* Controller UFSHCI version */ enum { UFSHCI_VERSION_10 = 0x00010000, /* 1.0 */

@@ -1022,6 +1031,8 @@ enum { writel((val), (hba)->mmio_base + (reg)) #define ufshcd_readl(hba, reg) \ readl((hba)->mmio_base + (reg)) +#define ufshcd_set_link_active(hba) ((hba)->uic_link_state = \

• 		    UIC_LINK_ACTIVE_STATE)
  

Same here, you set as active, but there's no users of uic_link_state

I think you can drop this

/**

• ufshcd_rmwl - perform read/modify/write for a controller register

diff --git a/drivers/ufs/ufshcd-dwc.c b/drivers/ufs/ufshcd-dwc.c new file mode 100644 index 0000000000..db51a3b807 --- /dev/null +++ b/drivers/ufs/ufshcd-dwc.c @@ -0,0 +1,141 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*

• • UFS Host driver for Synopsys Designware Core
• • Copyright (C) 2015-2016 Synopsys, Inc. (www.synopsys.com)
• */

+#include <clk.h> +#include <dm.h> +#include <ufs.h> +#include <asm/io.h> +#include <dm/device_compat.h> +#include <linux/bitops.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/time.h>

+#include "ufs.h" +#include "ufshci-dwc.h" +#include "ufshcd-dwc.h"

+int ufshcd_dwc_dme_set_attrs(struct ufs_hba *hba,

• 	     const struct ufshcd_dme_attr_val *v, int n)
  

+{

• int ret = 0;
• int attr_node = 0;
• for (attr_node = 0; attr_node < n; attr_node++) {

• ret = ufshcd_dme_set_attr(hba, v[attr_node].attr_sel,
  

• 			  ATTR_SET_NOR, v[attr_node].mib_val, v[attr_node].peer);
  

• if (ret)
  

• 	return ret;
  

• }
• return 0;

+}

+/**

• • ufshcd_dwc_program_clk_div() - program clock divider.
• • @hba: Private Structure pointer
• • @divider_val: clock divider value to be programmed
• */

+static void ufshcd_dwc_program_clk_div(struct ufs_hba *hba, u32 divider_val) +{

• ufshcd_writel(hba, divider_val, DWC_UFS_REG_HCLKDIV);

+}

+/**

• • ufshcd_dwc_link_is_up() - check if link is up.
• • @hba: private structure pointer
• • Return: 0 on success, non-zero value on failure.
• */

+static int ufshcd_dwc_link_is_up(struct ufs_hba *hba) +{

• int dme_result = 0;
• ufshcd_dme_get(hba, UIC_ARG_MIB(VS_POWERSTATE), &dme_result);
• if (dme_result == UFSHCD_LINK_IS_UP) {

• ufshcd_set_link_active(hba);
  

You can drop this, we don't track and use the link state

• return 0;
  

• }
• return 1;

+}

+/**

• • ufshcd_dwc_connection_setup() - configure unipro attributes.
• • @hba: pointer to drivers private data
• • This function configures both the local side (host) and the peer side
• • (device) unipro attributes to establish the connection to application/
• • cport.
• • This function is not required if the hardware is properly configured to
• • have this connection setup on reset. But invoking this function does no
• • harm and should be fine even working with any ufs device.
• • Return: 0 on success non-zero value on failure.
• */

+static int ufshcd_dwc_connection_setup(struct ufs_hba *hba) +{

• static const struct ufshcd_dme_attr_val setup_attrs[] = {

• { UIC_ARG_MIB(T_CONNECTIONSTATE), 0, DME_LOCAL },
  

• { UIC_ARG_MIB(N_DEVICEID), 0, DME_LOCAL },
  

• { UIC_ARG_MIB(N_DEVICEID_VALID), 0, DME_LOCAL },
  

• { UIC_ARG_MIB(T_PEERDEVICEID), 1, DME_LOCAL },
  

• { UIC_ARG_MIB(T_PEERCPORTID), 0, DME_LOCAL },
  

• { UIC_ARG_MIB(T_TRAFFICCLASS), 0, DME_LOCAL },
  

• { UIC_ARG_MIB(T_CPORTFLAGS), 0x6, DME_LOCAL },
  

• { UIC_ARG_MIB(T_CPORTMODE), 1, DME_LOCAL },
  

• { UIC_ARG_MIB(T_CONNECTIONSTATE), 1, DME_LOCAL },
  

• { UIC_ARG_MIB(T_CONNECTIONSTATE), 0, DME_PEER },
  

• { UIC_ARG_MIB(N_DEVICEID), 1, DME_PEER },
  

• { UIC_ARG_MIB(N_DEVICEID_VALID), 1, DME_PEER },
  

• { UIC_ARG_MIB(T_PEERDEVICEID), 1, DME_PEER },
  

• { UIC_ARG_MIB(T_PEERCPORTID), 0, DME_PEER },
  

• { UIC_ARG_MIB(T_TRAFFICCLASS), 0, DME_PEER },
  

• { UIC_ARG_MIB(T_CPORTFLAGS), 0x6, DME_PEER },
  

• { UIC_ARG_MIB(T_CPORTMODE), 1, DME_PEER },
  

• { UIC_ARG_MIB(T_CONNECTIONSTATE), 1, DME_PEER }
  

• };
• return ufshcd_dwc_dme_set_attrs(hba, setup_attrs, ARRAY_SIZE(setup_attrs));

+}

+/**

• • ufshcd_dwc_link_startup_notify() - program clock divider.
• • @hba: private structure pointer
• • @status: Callback notify status
• • Return: 0 on success, non-zero value on failure.
• */

+int ufshcd_dwc_link_startup_notify(struct ufs_hba *hba,

• 		   enum ufs_notify_change_status status)
  

+{

• int err = 0;
• if (status == PRE_CHANGE) {

• ufshcd_dwc_program_clk_div(hba, DWC_UFS_REG_HCLKDIV_DIV_125);
  

• err = ufshcd_vops_phy_initialization(hba);
  

• if (err) {
  

• 	dev_err(hba->dev, "Phy setup failed (%d)\n", err);
  

• 	return err;
  

• }
  

Drop this call, it's dead code, and weird to call core ops from a dwc helper...

• } else { /* POST_CHANGE */

• err = ufshcd_dwc_link_is_up(hba);
  

• if (err) {
  

• 	dev_err(hba->dev, "Link is not up\n");
  

• 	return err;
  

• }
  

• err = ufshcd_dwc_connection_setup(hba);
  

• if (err)
  

• 	dev_err(hba->dev, "Connection setup failed (%d)\n",
  

• 		err);
  

• }
• return err;

+} diff --git a/drivers/ufs/ufshcd-dwc.h b/drivers/ufs/ufshcd-dwc.h new file mode 100644 index 0000000000..d997045d3c --- /dev/null +++ b/drivers/ufs/ufshcd-dwc.h @@ -0,0 +1,23 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/*

• • UFS Host driver for Synopsys Designware Core
• • Copyright (C) 2015-2016 Synopsys, Inc. (www.synopsys.com)
• • Authors: Joao Pinto jpinto@synopsys.com
• */

+#ifndef _UFSHCD_DWC_H +#define _UFSHCD_DWC_H

+struct ufshcd_dme_attr_val {

• u32 attr_sel;
• u32 mib_val;
• u8 peer;

+};

+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);
  

+#endif /* End of Header */ diff --git a/drivers/ufs/ufshci-dwc.h b/drivers/ufs/ufshci-dwc.h new file mode 100644 index 0000000000..9e24c230c6 --- /dev/null +++ b/drivers/ufs/ufshci-dwc.h @@ -0,0 +1,32 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/*

• • UFS Host driver for Synopsys Designware Core
• • Copyright (C) 2015-2016 Synopsys, Inc. (www.synopsys.com)
• • Authors: Joao Pinto jpinto@synopsys.com
• */

+#ifndef _UFSHCI_DWC_H +#define _UFSHCI_DWC_H

+/* DWC HC UFSHCI specific Registers */ +enum dwc_specific_registers {

• DWC_UFS_REG_HCLKDIV = 0xFC,

+};

+/* Clock Divider Values: Hex equivalent of frequency in MHz */ +enum clk_div_values {

• DWC_UFS_REG_HCLKDIV_DIV_62_5 = 0x3e,
• DWC_UFS_REG_HCLKDIV_DIV_125 = 0x7d,
• DWC_UFS_REG_HCLKDIV_DIV_200 = 0xc8,

+};

+/* Selector Index */ +enum selector_index {

• SELIND_LN0_TX = 0x00,
• SELIND_LN1_TX = 0x01,
• SELIND_LN0_RX = 0x04,
• SELIND_LN1_RX = 0x05,

+}; +#endif

Thanks, Neil