scsi: ufs: set the device reference clock setting

From: Subhash Jadavani <[email protected]>

UFS host supplies the reference clock to UFS device and UFS device specification allows host to provide one of the 4 frequencies (19.2 MHz, 26 MHz, 38.4 MHz, 52 MHz) for reference clock. Host should set the device reference clock frequency setting in the device based on what frequency it is supplying to UFS device.

Signed-off-by Subhash Jadavani <[email protected]>

Signed-off-by Can Guo <[email protected]>

Signed-off-by Sayali Lokhande <[email protected]>

---
 .../devicetree/bindings/ufs/ufshcd-pltfrm.txt      |  7 +++
 drivers/scsi/ufs/ufs.h                             |  9 ++++
 drivers/scsi/ufs/ufshcd-pltfrm.c                   |  2 +
 drivers/scsi/ufs/ufshcd.c                          | 62 ++++++++++++++++++++++
 drivers/scsi/ufs/ufshcd.h                          |  2 +
 5 files changed, 82 insertions(+)

Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +0 -0

index c39dfef..bc20f59 100644

Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +1 -0

 Note: If above properties are not defined it can be assumed that the supply
 regulators or clocks are always on.

@@ -66,4 +72,5 @@ Example:
 		freq-table-hz = <100000000 200000000>, <0 0>, <0 0>;
 		phys = <&ufsphy1>;
 		phy-names = "ufsphy";
+		assigned-clock-rates = <0>; /* reference clock freq: 19.2 MHz */
 	};

drivers/scsi/ufs/ufs.h +9 -0

index 14e5bf7..e15deb0 100644
@@ -378,6 +378,15 @@ enum query_opcode {
 	UPIU_QUERY_OPCODE_TOGGLE_FLAG	= 0x8,
 };
 
+/* bRefClkFreq attribute values */
+enum ref_clk_freq {
+	REF_CLK_FREQ_19_2_MHZ	= 0x0,
+	REF_CLK_FREQ_26_MHZ	= 0x1,
+	REF_CLK_FREQ_38_4_MHZ	= 0x2,
+	REF_CLK_FREQ_52_MHZ	= 0x3,
+	REF_CLK_FREQ_MAX	= REF_CLK_FREQ_52_MHZ,
+};
+
 /* Query response result code */
 enum {
 	QUERY_RESULT_SUCCESS                    = 0x00,

drivers/scsi/ufs/ufshcd-pltfrm.c +2 -0

index e82bde0..0953563 100644
@@ -343,6 +343,8 @@ int ufshcd_pltfrm_init(struct platform_device *pdev,
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 
+	ufshcd_parse_dev_ref_clk_freq(hba);
+
 	ufshcd_init_lanes_per_dir(hba);
 
 	err = ufshcd_init(hba, mmio_base, irq);

drivers/scsi/ufs/ufshcd.c +42 -0

index c5b1bf1..455a6ad 100644
@@ -6296,6 +6296,62 @@ static void ufshcd_def_desc_sizes(struct ufs_hba *hba)
 	hba->desc_size.hlth_desc = QUERY_DESC_HEALTH_DEF_SIZE;
 }
 
+void ufshcd_parse_dev_ref_clk_freq(struct ufs_hba *hba)
+{
+	struct device *dev = hba->dev;
+	int ret;
+
+	if (!dev)
+		return;
+
+	ret = device_property_read_u32(dev, "assigned-clock-rates",
+				   &hba->dev_ref_clk_freq);
+	if (ret ||
+	    (hba->dev_ref_clk_freq > REF_CLK_FREQ_52_MHZ)) {
+		dev_err(hba->dev,
+		"%s: invalid ref_clk setting = %d\n",
+		__func__, hba->dev_ref_clk_freq);
+		hba->dev_ref_clk_freq = REF_CLK_FREQ_MAX + 1;
+	}
+}
+
+static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba)
+{
+	int err = 0;
+	int ref_clk = -1;
+	static const char * const ref_clk_freqs[] = {"19.2 MHz", "26 MHz",
+						     "38.4 MHz", "52 MHz"};
+
+	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+			QUERY_ATTR_IDN_REF_CLK_FREQ, 0, 0, &ref_clk);
+
+	if (err) {
+		dev_err(hba->dev, "%s: failed reading bRefClkFreq. err = %d\n",
+			 __func__, err);
+		goto out;
+	}
+
+	if (ref_clk == hba->dev_ref_clk_freq)
+		goto out; /* nothing to update */
+
+	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
+			QUERY_ATTR_IDN_REF_CLK_FREQ, 0, 0,
+			&hba->dev_ref_clk_freq);
+

Quoted 28 lines · Sayali Lokhande

From: Subhash Jadavani <[email protected]>

UFS host supplies the reference clock to UFS device and UFS device specification allows host to provide one of the 4 frequencies (19.2 MHz, 26 MHz, 38.4 MHz, 52 MHz) for reference clock. Host should set the device reference clock frequency setting in the device based on what frequency it is supplying to UFS device.

Signed-off-by Subhash Jadavani <[email protected]>

Signed-off-by Can Guo <[email protected]>

Signed-off-by Sayali Lokhande <[email protected]>

---
  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt      |  7 +++
  drivers/scsi/ufs/ufs.h                             |  9 ++++
  drivers/scsi/ufs/ufshcd-pltfrm.c                   |  2 +
  drivers/scsi/ufs/ufshcd.c                          | 62 ++++++++++++++++++++++
  drivers/scsi/ufs/ufshcd.h                          |  2 +
  5 files changed, 82 insertions(+)

Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +1 -0

index c39dfef..bc20f59 100644
@@ -41,6 +41,12 @@ Optional properties:
  -lanes-per-direction	: number of lanes available per direction - either 1 or 2.
  			  Note that it is assume same number of lanes is used both
  			  directions at once. If not specified, default is 2 lanes per direction.
+- assigned-clock-rates 	: Specify the device reference clock frequency, must be one of the following:

Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt Optional properties: · line 49

Quoted 5 lines

Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +5 -0

+			  0: 19.2 MHz
+			  1: 26 MHz
+			  2: 38.4 MHz
+			  3: 52 MHz
+			  Defaults to 26 MHz if not specified.

Sigh. You obviously did not read the definition of assigned-clock-rates and understand how it works. I don't think you want the frequency to be 0-3 Hz.

Quoted 98 lines

Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +1 -0

  Note: If above properties are not defined it can be assumed that the supply
  regulators or clocks are always on.

@@ -66,4 +72,5 @@ Example:
  		freq-table-hz = <100000000 200000000>, <0 0>, <0 0>;
  		phys = <&ufsphy1>;
  		phy-names = "ufsphy";
+		assigned-clock-rates = <0>; /* reference clock freq: 19.2 MHz */
  	};

drivers/scsi/ufs/ufs.h +9 -0

index 14e5bf7..e15deb0 100644
@@ -378,6 +378,15 @@ enum query_opcode {
  	UPIU_QUERY_OPCODE_TOGGLE_FLAG	= 0x8,
  };
 
+/* bRefClkFreq attribute values */
+enum ref_clk_freq {
+	REF_CLK_FREQ_19_2_MHZ	= 0x0,
+	REF_CLK_FREQ_26_MHZ	= 0x1,
+	REF_CLK_FREQ_38_4_MHZ	= 0x2,
+	REF_CLK_FREQ_52_MHZ	= 0x3,
+	REF_CLK_FREQ_MAX	= REF_CLK_FREQ_52_MHZ,
+};
+
  /* Query response result code */
  enum {
  	QUERY_RESULT_SUCCESS                    = 0x00,

drivers/scsi/ufs/ufshcd-pltfrm.c +2 -0

index e82bde0..0953563 100644
@@ -343,6 +343,8 @@ int ufshcd_pltfrm_init(struct platform_device *pdev,
  	pm_runtime_set_active(&pdev->dev);
  	pm_runtime_enable(&pdev->dev);
 
+	ufshcd_parse_dev_ref_clk_freq(hba);
+
  	ufshcd_init_lanes_per_dir(hba);
 
  	err = ufshcd_init(hba, mmio_base, irq);

drivers/scsi/ufs/ufshcd.c +49 -0

index c5b1bf1..455a6ad 100644
@@ -6296,6 +6296,62 @@ static void ufshcd_def_desc_sizes(struct ufs_hba *hba)
  	hba->desc_size.hlth_desc = QUERY_DESC_HEALTH_DEF_SIZE;
  }
 
+void ufshcd_parse_dev_ref_clk_freq(struct ufs_hba *hba)
+{
+	struct device *dev = hba->dev;
+	int ret;
+
+	if (!dev)
+		return;
+
+	ret = device_property_read_u32(dev, "assigned-clock-rates",
+				   &hba->dev_ref_clk_freq);
+	if (ret ||
+	    (hba->dev_ref_clk_freq > REF_CLK_FREQ_52_MHZ)) {
+		dev_err(hba->dev,
+		"%s: invalid ref_clk setting = %d\n",
+		__func__, hba->dev_ref_clk_freq);
+		hba->dev_ref_clk_freq = REF_CLK_FREQ_MAX + 1;
+	}
+}
+
+static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba)
+{
+	int err = 0;
+	int ref_clk = -1;
+	static const char * const ref_clk_freqs[] = {"19.2 MHz", "26 MHz",
+						     "38.4 MHz", "52 MHz"};
+
+	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+			QUERY_ATTR_IDN_REF_CLK_FREQ, 0, 0, &ref_clk);
+
+	if (err) {
+		dev_err(hba->dev, "%s: failed reading bRefClkFreq. err = %d\n",
+			 __func__, err);
+		goto out;
+	}
+
+	if (ref_clk == hba->dev_ref_clk_freq)
+		goto out; /* nothing to update */
+
+	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
+			QUERY_ATTR_IDN_REF_CLK_FREQ, 0, 0,
+			&hba->dev_ref_clk_freq);
+
+	if (err)
+		dev_err(hba->dev, "%s: bRefClkFreq setting to %s failed\n",
+			__func__, ref_clk_freqs[hba->dev_ref_clk_freq]);
+	/*
+	 * It is good to print this out here to debug any later failures
+	 * related to gear switch.
+	 */

drivers/scsi/ufs/ufshcd.c static void ufshcd_def_desc_sizes(struct ufs_hba *hba) · line 6350

Quoted 3 lines

drivers/scsi/ufs/ufshcd.c +3 -0

+	dev_dbg(hba->dev, "%s: bRefClkFreq setting to %s succeeded\n",
+			__func__, ref_clk_freqs[hba->dev_ref_clk_freq]);
+

I still don't understand how you think this is supposed to work. All this does is copy whatever freq setting you put into DT to this device attr. That does nothing to actually set the frequency of the ref_clk. It's frequency could be anything before and after this function.

Also, according to the spec, isn't QUERY_ATTR_IDN_REF_CLK_FREQ write-once? So if you can read the value, wouldn't that mean you can't set it.

Rob

This patch adds configfs support to provision ufs device at runtime. This feature can be primarily useful in factory or assembly line as some devices may be required to be configured multiple times during initial system development phase. Configuration Descriptors can be written multiple times until bConfigDescrLock attribute is zero.

Configuration descriptor buffer consists of Device and Unit descriptor configurable parameters which are parsed from vendor specific provisioning file and then passed via configfs node at runtime to provision ufs device.
CONFIG_CONFIGFS_FS needs to be enabled for using this feature.

Quoted 7 lines · Sayali Lokhande

This patch adds configfs support to provision ufs device at runtime. This feature can be primarily useful in factory or assembly line as some devices may be required to be configured multiple times during initial system development phase. Configuration Descriptors can be written multiple times until bConfigDescrLock attribute is zero.

Quoted 5 lines

Configuration descriptor buffer consists of Device and Unit descriptor configurable parameters which are parsed from vendor specific provisioning file and then passed via configfs node at runtime to provision ufs device.
CONFIG_CONFIGFS_FS needs to be enabled for using this feature.

As mentioned before: this absolutely needs to be a separate config option so that the code can be compiled out entirely.

Agree. Will add separate config for UFS Provision and set it dependent on CONFIGFS_FS

This patch adds configfs support to provision ufs device at

runtime. This feature can be primarily useful in factory or assembly line as some devices may be required to be configured multiple times during initial system development phase. Configuration Descriptors can be written multiple times until bConfigDescrLock attribute is zero.

Configuration descriptor buffer consists of Device and Unit descriptor configurable parameters which are parsed from vendor specific provisioning file and then passed via configfs node at runtime to provision ufs device.
CONFIG_CONFIGFS_FS needs to be enabled for using this feature.

Usage:
1) To read current configuration descriptor :
cat /config/ufshcd/ufs_provision
2) To provision ufs device:
echo <config_desc_buf> > /config/ufshcd/ufs_provision

Signed-off-by Sayali Lokhande <[email protected]>

---
 Documentation/ABI/testing/configfs-driver-ufs |  18 +++
 drivers/scsi/ufs/Makefile                     |   1 +
 drivers/scsi/ufs/ufs-configfs.c               | 172 ++++++++++++++++++++++++++
 drivers/scsi/ufs/ufshcd.c                     |   2 +
 drivers/scsi/ufs/ufshcd.h                     |  19 +++
 5 files changed, 212 insertions(+)
 create mode 100644 Documentation/ABI/testing/configfs-driver-ufs
 create mode 100644 drivers/scsi/ufs/ufs-configfs.c

Documentation/ABI/testing/configfs-driver-ufs new file +0 -0

new file mode 100644
index 0000000..dd16842

Documentation/ABI/testing/configfs-driver-ufs new file +1 -0

+               > /config/ufshcd/ufs_provision

drivers/scsi/ufs/Makefile +1 -0

index 918f579..d438e74 100644
@@ -5,5 +5,6 @@ obj-$(CONFIG_SCSI_UFS_DWC_TC_PLATFORM) += tc-dwc-g210-pltfrm.o ufshcd-dwc.o tc-d
 obj-$(CONFIG_SCSI_UFS_QCOM) += ufs-qcom.o
 obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o
 ufshcd-core-objs := ufshcd.o ufs-sysfs.o
+obj-$(CONFIG_CONFIGFS_FS) += ufs-configfs.o
 obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
 obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o

drivers/scsi/ufs/ufs-configfs.c new file +63 -0

new file mode 100644
index 0000000..7d207fc
@@ -0,0 +1,172 @@
+// SPDX-License-Identifier: GPL-2.0+
+// Copyright (c) 2018, Linux Foundation.
+
+#include <linux/configfs.h>
+#include <linux/err.h>
+#include <linux/string.h>
+
+#include "ufs.h"
+#include "ufshcd.h"
+
+static inline struct ufs_hba *config_item_to_hba(struct config_item *item)
+{
+       struct config_group *group = to_config_group(item);
+       struct configfs_subsystem *subsys = to_configfs_subsystem(group);
+       struct ufs_hba *hba = container_of(subsys, struct ufs_hba, subsys);
+
+       return hba ? hba : NULL;
+}
+
+static ssize_t ufs_provision_show(struct config_item *item, char *buf)
+{
+       u8 desc_buf[QUERY_DESC_CONFIGURATION_DEF_SIZE] = {0};
+       int buff_len = QUERY_DESC_CONFIGURATION_DEF_SIZE;
+       int i, ret, curr_len = 0;
+       struct ufs_hba *hba = config_item_to_hba(item);
+
+       if (!hba)
+               return snprintf(buf, PAGE_SIZE, "ufs hba is NULL!\n");
+
+       ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_READ_DESC,
+               QUERY_DESC_IDN_CONFIGURATION, 0, 0, desc_buf, &buff_len);
+
+       if (ret)
+               return snprintf(buf, PAGE_SIZE,
+               "Failed reading descriptor. desc_idn %d, opcode %x ret %d\n",
+               QUERY_DESC_IDN_CONFIGURATION,
+               UPIU_QUERY_OPCODE_READ_DESC, ret);
+
+       for (i = 0; i < buff_len; i++)
+               curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len),
+                               "0x%x ", desc_buf[i]);
+
+       return curr_len;
+}
+
+ssize_t ufshcd_desc_configfs_store(struct ufs_hba *hba,
+               const char *buf, size_t count)
+{
+       char *strbuf;
+       char *strbuf_copy;
+       u8 desc_buf[QUERY_DESC_CONFIGURATION_DEF_SIZE] = {0};
+       int buff_len = QUERY_DESC_CONFIGURATION_DEF_SIZE;
+       char *token;
+       int i, ret, value;
+       u32 bConfigDescrLock = 0;
+
+       /* reserve one byte for null termination */
+       strbuf = kmalloc(count + 1, GFP_KERNEL);
+       if (!strbuf)
+               return -ENOMEM;
+
+       strbuf_copy = strbuf;
+       strlcpy(strbuf, buf, count + 1);

drivers/scsi/ufs/ufs-configfs.c new file +10 -0

+       else
+               dev_err(hba->dev, "%s: UFS Provisioning done, reboot now!\n",
+               __func__);
+
+out:
+       kfree(strbuf_copy);
+       return count;
+}
+
+static ssize_t ufs_provision_store(struct config_item *item,

drivers/scsi/ufs/ufs-configfs.c new file +23 -0

+       .ca_owner       = THIS_MODULE,
+       .show           = ufs_provision_show,
+       .store          = ufs_provision_store,
+};
+
+static struct configfs_attribute *ufshcd_attrs[] = {
+       &ufshcd_attr_provision,
+       NULL,
+};
+
+static struct config_item_type ufscfg_type = {
+       .ct_attrs       = ufshcd_attrs,
+       .ct_owner       = THIS_MODULE,
+};
+
+static struct configfs_subsystem ufscfg_subsys = {
+       .su_group = {
+               .cg_item = {
+                       .ci_namebuf = "ufshcd",
+                       .ci_type = &ufscfg_type,
+               },
+       },
+};

Hi Sayali,
Thanks for the prompt spin.

Quoted 1 line · Sayali Lokhande

This patch adds configfs support to provision ufs device at

s/ufs/UFS/

Quoted 35 lines

runtime. This feature can be primarily useful in factory or assembly line as some devices may be required to be configured multiple times during initial system development phase. Configuration Descriptors can be written multiple times until bConfigDescrLock attribute is zero.

Configuration descriptor buffer consists of Device and Unit descriptor configurable parameters which are parsed from vendor specific provisioning file and then passed via configfs node at runtime to provision ufs device.
CONFIG_CONFIGFS_FS needs to be enabled for using this feature.

Usage:
1) To read current configuration descriptor :
cat /config/ufshcd/ufs_provision
2) To provision ufs device:
echo <config_desc_buf> > /config/ufshcd/ufs_provision

Signed-off-by Sayali Lokhande <[email protected]>

---
  Documentation/ABI/testing/configfs-driver-ufs |  18 +++
  drivers/scsi/ufs/Makefile                     |   1 +
  drivers/scsi/ufs/ufs-configfs.c               | 172 ++++++++++++++++++++++++++
  drivers/scsi/ufs/ufshcd.c                     |   2 +
  drivers/scsi/ufs/ufshcd.h                     |  19 +++
  5 files changed, 212 insertions(+)

create mode 100644 Documentation/ABI/testing/configfs-driver-ufs create mode 100644 drivers/scsi/ufs/ufs-configfs.c

Documentation/ABI/testing/configfs-driver-ufs new file +0 -0

new file mode 100644
index 0000000..dd16842

Documentation/ABI/testing/configfs-driver-ufs line 8

Quoted 8 lines

Documentation/ABI/testing/configfs-driver-ufs new file +8 -0

@@ -0,0 +1,18 @@
+What:          /config/ufshcd/ufs_provision
+Date:          Jun 2018
+KernelVersion: 4.14
+Description:
+               This file shows the current ufs configuration descriptor set in device.
+               This can be used to provision ufs device if bConfigDescrLock is 0.
+               For more details, refer 14.1.6.3 Configuration Descriptor and
+               Table 14-12 — Unit Descriptor configurable parameters from specs for

Can this be a regular dash, rather than some sort of exotic 0xE2 byte.

Documentation/ABI/testing/configfs-driver-ufs line 17

Quoted 9 lines

Documentation/ABI/testing/configfs-driver-ufs new file +9 -0

+               description of each configuration descriptor parameter.
+               Configuration descriptor buffer needs to be passed in space separated
+               format specificied as below:
+               echo <bLength> <bDescriptorIDN> <bConfDescContinue> <bBootEnable>
+               <bDescrAccessEn> <bInitPowerMode> <bHighPriorityLUN>
+               <bSecureRemovalType> <bInitActiveICCLevel> <wPeriodicRTCUpdate>
+               <0Bh:0Fh_ReservedAs_0> <bLUEnable> <bBootLunID> <bLUWriteProtect>
+               <bMemoryType> <dNumAllocUnits> <bDataReliability> <bLogicalBlockSize>
+               <bProvisioningType> <wContextCapabilities> <0Dh:0Fh_ReservedAs_0>

I wonder if at this point we should be using a binary attribute rather than a text one. Since now you're basically just converting text to bytes, it's not very human anymore.

Quoted 85 lines

Documentation/ABI/testing/configfs-driver-ufs new file +1 -0

+               > /config/ufshcd/ufs_provision

drivers/scsi/ufs/Makefile +1 -0

index 918f579..d438e74 100644
@@ -5,5 +5,6 @@ obj-$(CONFIG_SCSI_UFS_DWC_TC_PLATFORM) += tc-dwc-g210-pltfrm.o ufshcd-dwc.o tc-d
  obj-$(CONFIG_SCSI_UFS_QCOM) += ufs-qcom.o
  obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o
  ufshcd-core-objs := ufshcd.o ufs-sysfs.o
+obj-$(CONFIG_CONFIGFS_FS) += ufs-configfs.o
  obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
  obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o

drivers/scsi/ufs/ufs-configfs.c new file +67 -0

new file mode 100644
index 0000000..7d207fc
@@ -0,0 +1,172 @@
+// SPDX-License-Identifier: GPL-2.0+
+// Copyright (c) 2018, Linux Foundation.
+
+#include <linux/configfs.h>
+#include <linux/err.h>
+#include <linux/string.h>
+
+#include "ufs.h"
+#include "ufshcd.h"
+
+static inline struct ufs_hba *config_item_to_hba(struct config_item *item)
+{
+       struct config_group *group = to_config_group(item);
+       struct configfs_subsystem *subsys = to_configfs_subsystem(group);
+       struct ufs_hba *hba = container_of(subsys, struct ufs_hba, subsys);
+
+       return hba ? hba : NULL;
+}
+
+static ssize_t ufs_provision_show(struct config_item *item, char *buf)
+{
+       u8 desc_buf[QUERY_DESC_CONFIGURATION_DEF_SIZE] = {0};
+       int buff_len = QUERY_DESC_CONFIGURATION_DEF_SIZE;
+       int i, ret, curr_len = 0;
+       struct ufs_hba *hba = config_item_to_hba(item);
+
+       if (!hba)
+               return snprintf(buf, PAGE_SIZE, "ufs hba is NULL!\n");
+
+       ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_READ_DESC,
+               QUERY_DESC_IDN_CONFIGURATION, 0, 0, desc_buf, &buff_len);
+
+       if (ret)
+               return snprintf(buf, PAGE_SIZE,
+               "Failed reading descriptor. desc_idn %d, opcode %x ret %d\n",
+               QUERY_DESC_IDN_CONFIGURATION,
+               UPIU_QUERY_OPCODE_READ_DESC, ret);
+
+       for (i = 0; i < buff_len; i++)
+               curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len),
+                               "0x%x ", desc_buf[i]);
+
+       return curr_len;
+}
+
+ssize_t ufshcd_desc_configfs_store(struct ufs_hba *hba,
+               const char *buf, size_t count)
+{
+       char *strbuf;
+       char *strbuf_copy;
+       u8 desc_buf[QUERY_DESC_CONFIGURATION_DEF_SIZE] = {0};
+       int buff_len = QUERY_DESC_CONFIGURATION_DEF_SIZE;
+       char *token;
+       int i, ret, value;
+       u32 bConfigDescrLock = 0;
+
+       /* reserve one byte for null termination */
+       strbuf = kmalloc(count + 1, GFP_KERNEL);
+       if (!strbuf)
+               return -ENOMEM;
+
+       strbuf_copy = strbuf;
+       strlcpy(strbuf, buf, count + 1);
+
+       if (!hba)
+               goto out;
+

drivers/scsi/ufs/ufs-configfs.c line 73

Quoted 6 lines

drivers/scsi/ufs/ufs-configfs.c new file +6 -0

+       /* Just return if bConfigDescrLock is already set */
+       ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+               QUERY_ATTR_IDN_CONF_DESC_LOCK, 0, 0, &bConfigDescrLock);
+       if (ret) {
+               dev_err(hba->dev, "%s: Failed reading bConfigDescrLock %d, cannot re-provision device!\n",
+                       __func__, ret);

I think ufshcd_query_attr has its own prints on failure, we probably don't need this one.

drivers/scsi/ufs/ufs-configfs.c line 86

Quoted 13 lines

drivers/scsi/ufs/ufs-configfs.c new file +13 -0

+               goto out;
+       }
+       if (bConfigDescrLock) {
+               dev_err(hba->dev, "%s: bConfigDescrLock already set to %u, cannot re-provision device!\n",
+               __func__, bConfigDescrLock);
+               goto out;
+       }
+
+       for (i = 0; i < QUERY_DESC_CONFIGURATION_DEF_SIZE; i++) {
+               token = strsep(&strbuf, " ");
+               if (!token && i) {
+                       dev_dbg(hba->dev, "%s: token %s, i %d\n",
+                               __func__, token, i);

You're printing token, which you know to be null. Is this print really useful?

drivers/scsi/ufs/ufs-configfs.c line 97

Quoted 11 lines

drivers/scsi/ufs/ufs-configfs.c new file +11 -0

+                       break;
+               }
+
+               ret = kstrtoint(token, 0, &value);
+               if (ret) {
+                       dev_err(hba->dev, "%s: kstrtoint failed %d %s\n",
+                               __func__, ret, token);
+                       break;
+               }
+               desc_buf[i] = (u8)value;
+               dev_dbg(hba->dev, " desc_buf[%d] 0x%x", i, desc_buf[i]);

This print is probably a bit noisy. If you really want to dump the contents in the debug spew there's a print_hex_dump you can do after you break out of the loop. Then you could also remove the print for "token %s, i %d".

Quoted 4 lines

drivers/scsi/ufs/ufs-configfs.c new file +4 -0

+       }
+
+       /* Write configuration descriptor to provision ufs */
+       ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_WRITE_DESC,

drivers/scsi/ufs/ufs-configfs.c line 107

Quoted 6 lines

drivers/scsi/ufs/ufs-configfs.c new file +6 -0

+               QUERY_DESC_IDN_CONFIGURATION, 0, 0, desc_buf, &buff_len);
+
+       if (ret)
+               dev_err(hba->dev, "%s: Failed writing descriptor. desc_idn %d, opcode %x ret %d\n",
+               __func__, QUERY_DESC_IDN_CONFIGURATION,
+               UPIU_QUERY_OPCODE_WRITE_DESC, ret);

This is basically already covered by prints inside ufshcd_query_descriptor_retry.

Quoted 14 lines

drivers/scsi/ufs/ufs-configfs.c new file +14 -0

+       else
+               dev_err(hba->dev, "%s: UFS Provisioning done, reboot now!\n",
+               __func__);
+
+out:
+       kfree(strbuf_copy);
+       return count;
+}
+
+static ssize_t ufs_provision_store(struct config_item *item,
+               const char *buf, size_t count)
+{
+       struct ufs_hba *hba = config_item_to_hba(item);
+

drivers/scsi/ufs/ufs-configfs.c line 127

Quoted 6 lines

drivers/scsi/ufs/ufs-configfs.c new file +6 -0

+       return ufshcd_desc_configfs_store(hba, buf, count);
+}
+
+static struct configfs_attribute ufshcd_attr_provision = {
+       .ca_name        = "ufs_provision",
+       .ca_mode        = 0666,

This seems too permissive. You don't want just anybody to be able to re-provision the disk. Maybe 0644?

Quoted 31 lines

drivers/scsi/ufs/ufs-configfs.c new file +31 -0

+       .ca_owner       = THIS_MODULE,
+       .show           = ufs_provision_show,
+       .store          = ufs_provision_store,
+};
+
+static struct configfs_attribute *ufshcd_attrs[] = {
+       &ufshcd_attr_provision,
+       NULL,
+};
+
+static struct config_item_type ufscfg_type = {
+       .ct_attrs       = ufshcd_attrs,
+       .ct_owner       = THIS_MODULE,
+};
+
+static struct configfs_subsystem ufscfg_subsys = {
+       .su_group = {
+               .cg_item = {
+                       .ci_namebuf = "ufshcd",
+                       .ci_type = &ufscfg_type,
+               },
+       },
+};
+
+int ufshcd_configfs_init(struct ufs_hba *hba)
+{
+       int ret;
+       struct configfs_subsystem *subsys = &hba->subsys;
+
+       subsys->su_group = ufscfg_subsys.su_group;
+       config_group_init(&subsys->su_group);

drivers/scsi/ufs/ufs-configfs.c line 160

Quoted 2 lines

drivers/scsi/ufs/ufs-configfs.c new file +2 -0

+       mutex_init(&subsys->su_mutex);
+       ret = configfs_register_subsystem(subsys);

Wait so for every host controller, you register a subsystem called "ufshcd"? Won't that result in a naming conflict when the second controller comes along? I was expecting to see subsystem registration happen once, probably in a driver init function, and then some sort of device specific registration happen here. You'd need a unique name for each controller, maybe the device name itself?

-Evan

Hi Evan,

Quoted 6 lines · Evan Green

Hi Sayali,
Thanks for the prompt spin.

Quoted 1 line · Sayali Lokhande

This patch adds configfs support to provision ufs device at

s/ufs/UFS/

Will update.

Quoted 54 lines

Quoted 35 lines

runtime. This feature can be primarily useful in factory or assembly line as some devices may be required to be configured multiple times during initial system development phase. Configuration Descriptors can be written multiple times until bConfigDescrLock attribute is zero.

Configuration descriptor buffer consists of Device and Unit descriptor configurable parameters which are parsed from vendor specific provisioning file and then passed via configfs node at runtime to provision ufs device.
CONFIG_CONFIGFS_FS needs to be enabled for using this feature.

Usage:
1) To read current configuration descriptor :
cat /config/ufshcd/ufs_provision
2) To provision ufs device:
echo <config_desc_buf> > /config/ufshcd/ufs_provision

Signed-off-by Sayali Lokhande <[email protected]>

---
  Documentation/ABI/testing/configfs-driver-ufs |  18 +++
  drivers/scsi/ufs/Makefile                     |   1 +
  drivers/scsi/ufs/ufs-configfs.c               | 172 ++++++++++++++++++++++++++
  drivers/scsi/ufs/ufshcd.c                     |   2 +
  drivers/scsi/ufs/ufshcd.h                     |  19 +++
  5 files changed, 212 insertions(+)

create mode 100644 Documentation/ABI/testing/configfs-driver-ufs create mode 100644 drivers/scsi/ufs/ufs-configfs.c

Documentation/ABI/testing/configfs-driver-ufs new file +0 -0

new file mode 100644
index 0000000..dd16842

Documentation/ABI/testing/configfs-driver-ufs line 8

Quoted 8 lines

Documentation/ABI/testing/configfs-driver-ufs new file +8 -0

@@ -0,0 +1,18 @@
+What:          /config/ufshcd/ufs_provision
+Date:          Jun 2018
+KernelVersion: 4.14
+Description:
+               This file shows the current ufs configuration descriptor set in device.
+               This can be used to provision ufs device if bConfigDescrLock is 0.
+               For more details, refer 14.1.6.3 Configuration Descriptor and
+               Table 14-12 — Unit Descriptor configurable parameters from specs for

Can this be a regular dash, rather than some sort of exotic 0xE2 byte.

Quoted 9 lines

Documentation/ABI/testing/configfs-driver-ufs new file +9 -0

+               description of each configuration descriptor parameter.
+               Configuration descriptor buffer needs to be passed in space separated
+               format specificied as below:
+               echo <bLength> <bDescriptorIDN> <bConfDescContinue> <bBootEnable>
+               <bDescrAccessEn> <bInitPowerMode> <bHighPriorityLUN>
+               <bSecureRemovalType> <bInitActiveICCLevel> <wPeriodicRTCUpdate>
+               <0Bh:0Fh_ReservedAs_0> <bLUEnable> <bBootLunID> <bLUWriteProtect>
+               <bMemoryType> <dNumAllocUnits> <bDataReliability> <bLogicalBlockSize>
+               <bProvisioningType> <wContextCapabilities> <0Dh:0Fh_ReservedAs_0>

Quoted 3 lines

I wonder if at this point we should be using a binary attribute rather than a text one. Since now you're basically just converting text to bytes, it's not very human anymore.

Use of binary attr was ruled out before. Pasting previous comments from Greg :
"binary attributes are meant for hardware value pass-through" type stuff. Unless this is "raw" data straight from the hardware, binary does not work.

I see Bart has chimed in on the next series with a suggestion to break out each field into individual files within configfs. Bart, what are your feelings about converting to a binary attribute? I remember when I did my sysfs equivalent of this patch, somebody chimed in indicating a "commit" file might be needed so that the new configuration could be written in one fell swoop. One advantage of the binary attribute is that it writes the configuration atomically.

Quoted 7 lines · Evan Green

I see Bart has chimed in on the next series with a suggestion to break out each field into individual files within configfs. Bart, what are your feelings about converting to a binary attribute? I remember when I did my sysfs equivalent of this patch, somebody chimed in indicating a "commit" file might be needed so that the new configuration could be written in one fell swoop. One advantage of the binary attribute is that it writes the configuration atomically.

Hello Evan,

I may be missing some UFS background information. But since a configfs interface is being added I think the same rule applies as to all Linux kernel user space interfaces, namely that it should be backwards compatible. Additionally, if anyone ever will want to use this interface from a shell script, I think that it will be much easier to write multiple ASCII attributes than a single binary attribute.

Quoted 10 lines · Bart Van Assche

Quoted 7 lines · Evan Green

I see Bart has chimed in on the next series with a suggestion to break out each field into individual files within configfs. Bart, what are your feelings about converting to a binary attribute? I remember when I did my sysfs equivalent of this patch, somebody chimed in indicating a "commit" file might be needed so that the new configuration could be written in one fell swoop. One advantage of the binary attribute is that it writes the configuration atomically.

Quoted 9 lines

Hello Evan,

I may be missing some UFS background information. But since a configfs interface is being added I think the same rule applies as to all Linux kernel user space interfaces, namely that it should be backwards compatible. Additionally, if anyone ever will want to use this interface from a shell script, I think that it will be much easier to write multiple ASCII attributes than a single binary attribute.

Hi Bart,
I'm unsure about the compatibility aspect for binary attributes that essentially represent direct windows into hardware. I suppose this comes down to who this interface is most useful to. Hypothetically lets say a future revision of UFS adds fields to the configuration descriptor, but is otherwise backwards compatible. If this interface is primarily for OEMs initializing their devices in the factory, then I'd argue they'd want the most direct window to the configuration descriptor. These folks probably just have a configuration they want to plunk into the hardware, and would prefer being able to write all fields over having some sort of compatibility restriction. If, on the other hand, this is used by long-running scripts that stick around for years without modification, then yes, it seems like it would be more important to stay compatible, and have smarts in the kernel to make writes of old descriptors work in new devices.

At least for myself, I fall into the category of someone who just needs to plunk a configuration descriptor in once, and would prefer not to have to submit kernel changes if the descriptor evolves slightly. It also seemed a little odd that this patch now spends a bunch of energy converting ASCII into bytes, just to write it without modification into the hardware, and convert back again to ASCII for reads.

We plan to use a script for provisioning, and could easily handle ASCII or rawbytes:

# Some bytes, ready to go with the interface today... some_bytes="00 01 02 03"

# Same bytes, now in binary format bytes_fmt=$(echo " $some_bytes" | sed 's/ /\\x/g') /usr/bin/printf "$bytes_fmt" > /configfs/ufs_provision

I'm not dead set on binary, since as above I could do it either way, but it seemed worth at least talking through. Let me know what you think.

Quoted 55 lines · Evan Green

Quoted 10 lines · Bart Van Assche

Quoted 7 lines · Evan Green

I see Bart has chimed in on the next series with a suggestion to break out each field into individual files within configfs. Bart, what are your feelings about converting to a binary attribute? I remember when I did my sysfs equivalent of this patch, somebody chimed in indicating a "commit" file might be needed so that the new configuration could be written in one fell swoop. One advantage of the binary attribute is that it writes the configuration atomically.

Quoted 9 lines

Hello Evan,

I may be missing some UFS background information. But since a configfs interface is being added I think the same rule applies as to all Linux kernel user space interfaces, namely that it should be backwards compatible. Additionally, if anyone ever will want to use this interface from a shell script, I think that it will be much easier to write multiple ASCII attributes than a single binary attribute.

Hi Bart,
I'm unsure about the compatibility aspect for binary attributes that essentially represent direct windows into hardware. I suppose this comes down to who this interface is most useful to. Hypothetically lets say a future revision of UFS adds fields to the configuration descriptor, but is otherwise backwards compatible. If this interface is primarily for OEMs initializing their devices in the factory, then I'd argue they'd want the most direct window to the configuration descriptor. These folks probably just have a configuration they want to plunk into the hardware, and would prefer being able to write all fields over having some sort of compatibility restriction. If, on the other hand, this is used by long-running scripts that stick around for years without modification, then yes, it seems like it would be more important to stay compatible, and have smarts in the kernel to make writes of old descriptors work in new devices.

At least for myself, I fall into the category of someone who just needs to plunk a configuration descriptor in once, and would prefer not to have to submit kernel changes if the descriptor evolves slightly. It also seemed a little odd that this patch now spends a bunch of energy converting ASCII into bytes, just to write it without modification into the hardware, and convert back again to ASCII for reads.

We plan to use a script for provisioning, and could easily handle ASCII or rawbytes:

# Some bytes, ready to go with the interface today... some_bytes="00 01 02 03"

# Same bytes, now in binary format bytes_fmt=$(echo " $some_bytes" | sed 's/ /\\x/g') /usr/bin/printf "$bytes_fmt" > /configfs/ufs_provision

Quoted 3 lines

I'm not dead set on binary, since as above I could do it either way, but it seemed worth at least talking through. Let me know what you think.

The configfs documentation (Documentation/filesystems/configfs/configfs.txt) is clear about this: "Preferably only one value per file should be used." So I would like to hear the opinion of someone who has more authority than I with regard to configfs.

Quoted 5 lines · Bart Van Assche

Quoted 3 lines · Evan Green

I'm not dead set on binary, since as above I could do it either way, but it seemed worth at least talking through. Let me know what you think.

Quoted 4 lines

The configfs documentation (Documentation/filesystems/configfs/configfs.txt) is clear about this: "Preferably only one value per file should be used." So I would like to hear the opinion of someone who has more authority than I with regard to configfs.

Don't we have "binary" files for configfs? We have them for sysfs, they are for files that are not touched by the kernel and just "pass-through" to the hardware. Would that work here as well?

Quoted 11 lines · On Wed, 2018-07-18 at 10:56 +0200, [email protected] wrote:

Quoted 9 lines · Bart Van Assche

Quoted 3 lines · Evan Green

I'm not dead set on binary, since as above I could do it either way, but it seemed worth at least talking through. Let me know what you think.

The configfs documentation (Documentation/filesystems/configfs/configfs.txt) is clear about this: "Preferably only one value per file should be used." So I would like to hear the opinion of someone who has more authority than I with regard to configfs.

Quoted 3 lines

Don't we have "binary" files for configfs? We have them for sysfs, they are for files that are not touched by the kernel and just "pass-through" to the hardware. Would that work here as well?

If a new version of the UFS spec would be introduced and that new version of the spec introduces a new layout for the binary descriptor, will it be possible for user space software to figure out which version of the binary descriptor format that has to be used?

Quoted 8 lines · Bart Van Assche

Quoted 7 lines · Evan Green

I see Bart has chimed in on the next series with a suggestion to break out each field into individual files within configfs. Bart, what are your feelings about converting to a binary attribute? I remember when I did my sysfs equivalent of this patch, somebody chimed in indicating a "commit" file might be needed so that the new configuration could be written in one fell swoop. One advantage of the binary attribute is that it writes the configuration atomically.

Quoted 9 lines

Hello Evan,

I may be missing some UFS background information. But since a configfs interface is being added I think the same rule applies as to all Linux kernel user space interfaces, namely that it should be backwards compatible. Additionally, if anyone ever will want to use this interface from a shell script, I think that it will be much easier to write multiple ASCII attributes than a single binary attribute.

Hi Bart,
I'm unsure about the compatibility aspect for binary attributes that essentially represent direct windows into hardware. I suppose this comes down to who this interface is most useful to. Hypothetically lets say a future revision of UFS adds fields to the configuration descriptor, but is otherwise backwards compatible. If this interface is primarily for OEMs initializing their devices in the factory, then I'd argue they'd want the most direct window to the configuration descriptor. These folks probably just have a configuration they want to plunk into the hardware, and would prefer being able to write all fields over having some sort of compatibility restriction. If, on the other hand, this is used by long-running scripts that stick around for years without modification, then yes, it seems like it would be more important to stay compatible, and have smarts in the kernel to make writes of old descriptors work in new devices.

At least for myself, I fall into the category of someone who just needs to plunk a configuration descriptor in once, and would prefer not to have to submit kernel changes if the descriptor evolves slightly. It also seemed a little odd that this patch now spends a bunch of energy converting ASCII into bytes, just to write it without modification into the hardware, and convert back again to ASCII for reads.

We plan to use a script for provisioning, and could easily handle ASCII or rawbytes:

# Some bytes, ready to go with the interface today... some_bytes="00 01 02 03"

# Same bytes, now in binary format bytes_fmt=$(echo " $some_bytes" | sed 's/ /\\x/g') /usr/bin/printf "$bytes_fmt" > /configfs/ufs_provision

I'm not dead set on binary, since as above I could do it either way, but it seemed worth at least talking through. Let me know what you think.
-Evan

Hi Evan,

On 7/18/2018 1:53 AM, Evan Green wrote:

Quoted 4 lines

I'm not dead set on binary, since as above I could do it either way, but it seemed worth at least talking through. Let me know what you think.
-Evan

I am using ASCII format for reading/writing to config desc as it will be more readable for users and easier/comfortable to compare any value to default spec value(if required).
So I don't really see any harm in using current ASCII format for provisioning purpose.

Quoted 4 lines · Sayali Lokhande

Hi Evan,

On 7/18/2018 1:53 AM, Evan Green wrote:

...

Quoted 9 lines

Quoted 4 lines

I'm not dead set on binary, since as above I could do it either way, but it seemed worth at least talking through. Let me know what you think.
-Evan

I am using ASCII format for reading/writing to config desc as it will be more readable for users and easier/comfortable to compare any value to default spec value(if required).
So I don't really see any harm in using current ASCII format for provisioning purpose.

I'm not convinced by those arguments, but ultimately it's between you and the maintainers. If you're going with the ASCII route, then I have another review comment. Currently in your patch, if kstrtoint fails, you print, but then break out of the loop and try to write the partially parsed descriptor anyway. That "break" should probably be changed to a "goto out".

-Evan