Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 5A9B960261
From:   "sayali" <sayalil@codeaurora.org>
To:     "'Kyuho Choi'" <chlrbgh0@gmail.com>
Cc:     <subhashj@codeaurora.org>, <cang@codeaurora.org>,
        <vivek.gautam@codeaurora.org>, <rnayak@codeaurora.org>,
        <vinholikatti@gmail.com>, <jejb@linux.vnet.ibm.com>,
        <martin.petersen@oracle.com>, <asutoshd@codeaurora.org>,
        <evgreen@chromium.org>, <linux-scsi@vger.kernel.org>,
        "'open list'" <linux-kernel@vger.kernel.org>
References: <1527849774-7623-1-git-send-email-sayalil@codeaurora.org> <1527849774-7623-3-git-send-email-sayalil@codeaurora.org> <CAP2JTQ+F0NBrTL0hOrdW0n88zpvCY+x1Fh6WZ0wm_xnK1Vn0Nw@mail.gmail.com>
In-Reply-To: <CAP2JTQ+F0NBrTL0hOrdW0n88zpvCY+x1Fh6WZ0wm_xnK1Vn0Nw@mail.gmail.com>
Subject: RE: [PATCH V1 2/3] scsi: ufs: Add ufs provisioning support
Date:   Tue, 5 Jun 2018 16:24:33 +0530
Message-ID: <000a01d3fcbb$99c254d0$cd46fe70$@codeaurora.org>
MIME-Version: 1.0
Content-Type: text/plain;
        charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Thread-Index: AQKGja9h2+sAa9B7VDYjBpPLWNjYigHtkbPiAq5DGKCix3uVwA==
Content-Language: en-us
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


-----Original Message-----
From: Kyuho Choi [mailto:chlrbgh0@gmail.com]=20
Sent: Saturday, June 02, 2018 12:06 PM
To: Sayali Lokhande <sayalil@codeaurora.org>
Cc: subhashj@codeaurora.org; cang@codeaurora.org; =
vivek.gautam@codeaurora.org; rnayak@codeaurora.org; =
vinholikatti@gmail.com; jejb@linux.vnet.ibm.com; =
martin.petersen@oracle.com; asutoshd@codeaurora.org; =
evgreen@chromium.org; linux-scsi@vger.kernel.org; open list =
<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH V1 2/3] scsi: ufs: Add ufs provisioning support

Sayali,

On 6/1/18, Sayali Lokhande <sayalil@codeaurora.org> wrote:
> A new api ufshcd_do_config_device() is added in driver to suppoet UFS=20
> provisioning at runtime. Sysfs support is added to trigger=20
> provisioning.
> Device and Unit configurable parameters are parsed from vendor=20
> specific provisioning data or file and passed via sysfs at runtime to=20
> provision ufs device.
>
> Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufs.h    |  28 +++++++
>  drivers/scsi/ufs/ufshcd.c | 200
> ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/scsi/ufs/ufshcd.h |   1 +
>  3 files changed, 229 insertions(+)
>
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h index=20
> e15deb0..1f99904 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -333,6 +333,7 @@ enum {
>  	UFSHCD_AMP		=3D 3,
>  };
>
> +#define UFS_BLOCK_SIZE	4096
>  #define POWER_DESC_MAX_SIZE			0x62
>  #define POWER_DESC_MAX_ACTV_ICC_LVLS		16
>
> @@ -425,6 +426,33 @@ enum {
>  	MASK_TM_SERVICE_RESP		=3D 0xFF,
>  };
>
> +struct ufs_unit_desc {
> +	u8     bLUEnable;              /* 1 for enabled LU */
> +	u8     bBootLunID;             /* 0 for using this LU for boot */
> +	u8     bLUWriteProtect;        /* 1 =3D power on WP, 2 =3D permanent =
WP */
> +	u8     bMemoryType;            /* 0 for enhanced memory type */
> +	u32    dNumAllocUnits;         /* Number of alloc unit for this LU =
*/
> +	u8     bDataReliability;       /* 0 for reliable write support */
> +	u8     bLogicalBlockSize;      /* See section 13.2.3 of UFS standard =
*/
> +	u8     bProvisioningType;      /* 0 for thin provisioning */
> +	u16    wContextCapabilities;   /* refer Unit Descriptor Description =
*/
> +};
> +
> +struct ufs_config_descr {
> +	u8     bNumberLU;              /* Total number of active LUs */
> +	u8     bBootEnable;            /* enabling device for partial init =
*/
> +	u8     bDescrAccessEn;         /* desc access during partial init */
> +	u8     bInitPowerMode;         /* Initial device power mode */
> +	u8     bHighPriorityLUN;       /* LUN of the high priority LU */
> +	u8     bSecureRemovalType;     /* Erase config for data removal */
> +	u8     bInitActiveICCLevel;    /* ICC level after reset */
> +	u16    wPeriodicRTCUpdate;     /* 0 to set a priodic RTC update rate =
*/
> +	u32     bConfigDescrLock;      /* 1 to lock Configation Descriptor =
*/
> +	u32    qVendorConfigCode;      /* Vendor specific configuration code =
*/
> +	struct ufs_unit_desc unit[8];
> +	u8	lun_to_grow;
> +};
> +
>  /* Task management service response */  enum {
>  	UPIU_TASK_MANAGEMENT_FUNC_COMPL		=3D 0x00,
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c=20
> index 3669bc4..3fd29e0 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -237,6 +237,7 @@ static int ufshcd_config_pwr_mode(struct ufs_hba =
*hba,
>  		struct ufs_pa_layer_attr *desired_pwr_mode);  static int=20
> ufshcd_change_power_mode(struct ufs_hba *hba,
>  			     struct ufs_pa_layer_attr *pwr_mode);
> +static int ufshcd_do_config_device(struct ufs_hba *hba);
>  static inline bool ufshcd_valid_tag(struct ufs_hba *hba, int tag)  {
>  	return tag >=3D 0 && tag < hba->nutrs; @@ -3063,6 +3064,14 @@ static =

> inline int ufshcd_read_power_desc(struct ufs_hba *hba,
>  	return ufshcd_read_desc(hba, QUERY_DESC_IDN_POWER, 0, buf, size);  }
>
> +static inline int ufshcd_read_geometry_desc(struct ufs_hba *hba,
> +					 u8 *buf,
> +					 u32 size)
> +{
> +	return ufshcd_read_desc(hba, QUERY_DESC_IDN_GEOMETRY, 0, buf, size); =

> +}
> +
> +
>  static int ufshcd_read_device_desc(struct ufs_hba *hba, u8 *buf, u32=20
> size)  {
>  	return ufshcd_read_desc(hba, QUERY_DESC_IDN_DEVICE, 0, buf, size);=20
> @@ -6354,6 +6363,197 @@ static int ufshcd_set_dev_ref_clk(struct=20
> ufs_hba *hba, u32 ref_clk_freq)  }
>
>  /**
> + * ufshcd_do_config_device - API function for UFS provisioning
> + * hba: per-adapter instance
> + * Returns 0 for success, non-zero in case of failure.
> + */
> +static int ufshcd_do_config_device(struct ufs_hba *hba) {
> +	struct ufs_config_descr *cfg =3D &hba->cfgs;
> +	int buff_len =3D QUERY_DESC_CONFIGURATION_DEF_SIZE;
> +	u8 desc_buf[QUERY_DESC_CONFIGURATION_DEF_SIZE] =3D {0};
> +	int i, ret =3D 0;
> +	int lun_to_grow =3D -1;
> +	u64 qTotalRawDeviceCapacity;
> +	u16 wEnhanced1CapAdjFac, wEnhanced2CapAdjFac;
> +	u32 dEnhanced1MaxNAllocU, dEnhanced2MaxNAllocU;
> +	size_t alloc_units, units_to_create =3D 0;
> +	size_t capacity_to_alloc_factor;
> +	size_t enhanced1_units =3D 0, enhanced2_units =3D 0;
> +	size_t conversion_ratio =3D 1;
> +	u8 *pt;
> +	u32 blocks_per_alloc_unit =3D 1024;
> +	int geo_len =3D hba->desc_size.geom_desc;
> +	u8 geo_buf[hba->desc_size.geom_desc];
> +	unsigned int max_partitions =3D 8;
> +
> +	WARN_ON(!hba || !cfg);
> +	ufshcd_set_dev_ref_clk(hba, REF_CLK_FREQ_19_2_MHZ);

Why you try to change the device refclk in here?. You need to check =
about ufs power mode before change the ufs device's refclk. The refclk =
change function wokring fine even when the ufs device in HS mode?. As I =
understood, refclk change in HS are not guaranteed by MPHY spec.
[Sayali] Agreed. I need not again set ref_clk here before runtime =
provisioning every time, as I am setting ref_clk during probe in =
ufshcd_probe_hba() .=20
It anyway does nothing here as ref_clk in device will be already set =
before, so it just returns. Will remove setting ref_clk from here in my =
next patchset.=20

> +
> +	ret =3D ufshcd_read_geometry_desc(hba, geo_buf, geo_len);
> +	if (ret) {
> +		dev_err(hba->dev, "%s: Failed getting geometry_desc %d\n",
> +			__func__, ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Get Geomtric parameters like total configurable memory
> +	 * quantity (Offset 0x04 to 0x0b), Capacity Adjustment
> +	 * Factors (Offset 0x30, 0x31, 0x36, 0x37), Max allocation
> +	 * units (Offset 0x2c to 0x2f, 0x32 to 0x35) used to configure
> +	 * the device logical units.
> +	 */
> +	qTotalRawDeviceCapacity =3D
> +		(uint64_t)geo_buf[0x0b] | ((uint64_t)geo_buf[0x0a] << 8) |
> +		((uint64_t)geo_buf[0x09] << 16) |
> +		((uint64_t)geo_buf[0x08] << 24) |
> +		((uint64_t)geo_buf[0x07] << 32) |
> +		((uint64_t)geo_buf[0x06] << 40) |
> +		((uint64_t)geo_buf[0x05] << 48) |
> +		((uint64_t)geo_buf[0x04] << 56);
> +	wEnhanced1CapAdjFac =3D
> +		(uint16_t)geo_buf[0x31] | ((uint16_t)geo_buf[0x30] << 8);
> +	wEnhanced2CapAdjFac =3D
> +		(uint16_t)geo_buf[0x37] | ((uint16_t)geo_buf[0x36] << 8);
> +	dEnhanced1MaxNAllocU =3D
> +		(uint32_t)geo_buf[0x2f] | ((uint32_t)geo_buf[0x2e] << 8) |
> +		((uint32_t)geo_buf[0x2d] << 16) |
> +		((uint32_t)geo_buf[0x2c] << 24);
> +	dEnhanced2MaxNAllocU =3D
> +		(uint32_t)geo_buf[0x35] | ((uint32_t)geo_buf[0x34] << 8) |
> +		((uint32_t)geo_buf[0x33] << 16) |
> +		((uint32_t)geo_buf[0x32] << 24);
> +
> +	capacity_to_alloc_factor =3D
> +		(blocks_per_alloc_unit * UFS_BLOCK_SIZE) / 512;
> +
> +	if (qTotalRawDeviceCapacity % capacity_to_alloc_factor !=3D 0) {
> +		dev_err(hba->dev,
> +			"%s: Raw capacity(%llu) not multiple of alloc factor(%zu)\n",
> +			__func__, qTotalRawDeviceCapacity,
> +			capacity_to_alloc_factor);
> +		return -EINVAL;
> +	}
> +	alloc_units =3D (qTotalRawDeviceCapacity / =
capacity_to_alloc_factor);
> +	units_to_create =3D 0;
> +	enhanced1_units =3D enhanced2_units =3D 0;
> +
> +	/*
> +	 * Calculate number of allocation units to be assigned to a logical =
unit
> +	 * considering the capacity adjustment factor of respective memory =
type.
> +	 */
> +	for (i =3D 0; i < (max_partitions - 1) &&
> +		units_to_create <=3D alloc_units; i++) {
> +		if ((cfg->unit[i].dNumAllocUnits % blocks_per_alloc_unit) =3D=3D 0)
> +			cfg->unit[i].dNumAllocUnits /=3D blocks_per_alloc_unit;
> +		else
> +			cfg->unit[i].dNumAllocUnits =3D
> +			cfg->unit[i].dNumAllocUnits / blocks_per_alloc_unit + 1;
> +
> +		if (cfg->unit[i].bMemoryType =3D=3D 0)
> +			units_to_create +=3D cfg->unit[i].dNumAllocUnits;
> +		else if (cfg->unit[i].bMemoryType =3D=3D 3) {
> +			enhanced1_units +=3D cfg->unit[i].dNumAllocUnits;
> +			cfg->unit[i].dNumAllocUnits *=3D
> +				(wEnhanced1CapAdjFac / 0x100);
> +			units_to_create +=3D cfg->unit[i].dNumAllocUnits;
> +		} else if (cfg->unit[i].bMemoryType =3D=3D 4) {
> +			enhanced2_units +=3D cfg->unit[i].dNumAllocUnits;
> +			cfg->unit[i].dNumAllocUnits *=3D
> +				(wEnhanced1CapAdjFac / 0x100);
> +			units_to_create +=3D cfg->unit[i].dNumAllocUnits;
> +		} else {
> +			dev_err(hba->dev, "%s: Unsupported memory type %d\n",
> +				__func__, cfg->unit[i].bMemoryType);
> +			return -EINVAL;
> +		}
> +	}
> +	if (enhanced1_units > dEnhanced1MaxNAllocU) {
> +		dev_err(hba->dev, "%s: size %zu exceeds max enhanced1 area size =
%u\n",
> +			__func__, enhanced1_units, dEnhanced1MaxNAllocU);
> +		return -ERANGE;
> +	}
> +	if (enhanced2_units > dEnhanced2MaxNAllocU) {
> +		dev_err(hba->dev, "%s: size %zu exceeds max enhanced2 area size =
%u\n",
> +			__func__, enhanced2_units, dEnhanced2MaxNAllocU);
> +		return -ERANGE;
> +	}
> +	if (units_to_create > alloc_units) {
> +		dev_err(hba->dev, "%s: Specified size %zu exceeds device size =
%zu\n",
> +			__func__, units_to_create, alloc_units);
> +		return -ERANGE;
> +	}
> +	lun_to_grow =3D cfg->lun_to_grow;
> +	if (lun_to_grow !=3D -1) {
> +		if (cfg->unit[i].bMemoryType =3D=3D 0)
> +			conversion_ratio =3D 1;
> +		else if (cfg->unit[i].bMemoryType =3D=3D 3)
> +			conversion_ratio =3D (wEnhanced1CapAdjFac / 0x100);
> +		else if (cfg->unit[i].bMemoryType =3D=3D 4)
> +			conversion_ratio =3D (wEnhanced2CapAdjFac / 0x100);
> +
> +		cfg->unit[lun_to_grow].dNumAllocUnits +=3D
> +		((alloc_units - units_to_create) / conversion_ratio);
> +		dev_dbg(hba->dev, "%s: conversion_ratio %zu for lun %d\n",
> +			__func__, conversion_ratio, i);
> +	}
> +
> +	/* Fill in the buffer with configuration data */
> +	pt =3D desc_buf;
> +	*pt++ =3D 0x90;        // bLength
> +	*pt++ =3D 0x01;        // bDescriptorType
> +	*pt++ =3D 0;           // Reserved in UFS2.0 and onward
> +	*pt++ =3D cfg->bBootEnable;
> +	*pt++ =3D cfg->bDescrAccessEn;
> +	*pt++ =3D cfg->bInitPowerMode;
> +	*pt++ =3D cfg->bHighPriorityLUN;
> +	*pt++ =3D cfg->bSecureRemovalType;
> +	*pt++ =3D cfg->bInitActiveICCLevel;
> +	*pt++ =3D (cfg->wPeriodicRTCUpdate >> 8) & 0xff;
> +	*pt++ =3D cfg->wPeriodicRTCUpdate & 0xff;
> +	pt =3D pt + 5; // Reserved fields set to 0
> +
> +	/* Fill in the buffer with per logical unit data */
> +	for (i =3D 0; i < UFS_UPIU_MAX_GENERAL_LUN; i++) {
> +		*pt++ =3D cfg->unit[i].bLUEnable;
> +		*pt++ =3D cfg->unit[i].bBootLunID;
> +		*pt++ =3D cfg->unit[i].bLUWriteProtect;
> +		*pt++ =3D cfg->unit[i].bMemoryType;
> +		*pt++ =3D (cfg->unit[i].dNumAllocUnits >> 24) & 0xff;
> +		*pt++ =3D (cfg->unit[i].dNumAllocUnits >> 16) & 0xff;
> +		*pt++ =3D (cfg->unit[i].dNumAllocUnits >> 8) & 0xff;
> +		*pt++ =3D (cfg->unit[i].dNumAllocUnits) & 0xff;
> +		*pt++ =3D cfg->unit[i].bDataReliability;
> +		*pt++ =3D cfg->unit[i].bLogicalBlockSize;
> +		*pt++ =3D cfg->unit[i].bProvisioningType;
> +		*pt++ =3D (cfg->unit[i].wContextCapabilities >> 8) & 0xff;
> +		*pt++ =3D cfg->unit[i].wContextCapabilities;
> +		pt =3D pt + 3; // Reserved fields set to 0
> +	}
> +
> +	ret =3D ufshcd_query_descriptor_retry(hba, =
UPIU_QUERY_OPCODE_WRITE_DESC,
> +		QUERY_DESC_IDN_CONFIGURATION, 0, 0, desc_buf, &buff_len);
> +
> +	if (ret) {
> +		dev_err(hba->dev, "%s: Failed writing descriptor. desc_idn %d,=20
> +opcode %x
> ret %d\n",
> +		__func__, QUERY_DESC_IDN_CONFIGURATION,
> +		UPIU_QUERY_OPCODE_WRITE_DESC, ret);
> +		return ret;
> +	}
> +
> +	if (cfg->bConfigDescrLock =3D=3D 1) {
> +		ret =3D ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> +		QUERY_ATTR_IDN_CONF_DESC_LOCK, 0, 0, &cfg->bConfigDescrLock);
> +		if (ret)
> +			dev_err(hba->dev, "%s: Failed writing bConfigDescrLock %d\n",
> +				__func__, ret);
> +	}
> +
> +	return ret;
> +}
> +
> +/**
>   * ufshcd_probe_hba - probe hba to detect device and initialize
>   * @hba: per-adapter instance
>   *
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h=20
> index b026ad8..982bfdf 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -549,6 +549,7 @@ struct ufs_hba {
>  	unsigned int irq;
>  	bool is_irq_enabled;
>  	u32 dev_ref_clk_freq;
> +	struct ufs_config_descr cfgs;
>
>  	/* Interrupt aggregation support is broken */
>  	#define UFSHCD_QUIRK_BROKEN_INTR_AGGR			0x1
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora=20
> Forum, a Linux Foundation Collaborative Project
>
>