Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <2f5a8742-b25d-d6b3-c6a4-1c119a94d569@acm.org>
Date:   Fri, 30 Sep 2022 11:23:13 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.0
Subject: Re: [PATCH v1 01/16] ufs: core: Probe for ext_iid support
Content-Language: en-US
To:     Asutosh Das <quic_asutoshd@quicinc.com>, mani@kernel.org,
        quic_nguyenb@quicinc.com, quic_xiaosenh@quicinc.com,
        quic_cang@quicinc.com, quic_nitirawa@quicinc.com,
        quic_rampraka@quicinc.com, quic_richardp@quicinc.com,
        stanley.chu@mediatek.com, adrian.hunter@intel.com,
        avri.altman@wdc.com, beanhuo@micron.com, martin.petersen@oracle.com
Cc:     linux-scsi@vger.kernel.org, Alim Akhtar <alim.akhtar@samsung.com>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        Jinyoung Choi <j-young.choi@samsung.com>,
        Kiwoong Kim <kwmad.kim@samsung.com>,
        open list <linux-kernel@vger.kernel.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-arm-kernel@lists.infradead.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@lists.infradead.org>
References: <cover.1663894792.git.quic_asutoshd@quicinc.com>
 <ff7cdcdef82f6c9d709e2b6ed5d91e255327b780.1663894792.git.quic_asutoshd@quicinc.com>
From:   Bart Van Assche <bvanassche@acm.org>
In-Reply-To: <ff7cdcdef82f6c9d709e2b6ed5d91e255327b780.1663894792.git.quic_asutoshd@quicinc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 9/22/22 18:05, Asutosh Das wrote:
> +	hba->mcq_capabilities = ufshcd_readl(hba, REG_MCQCAP);

What value is reported when reading the REG_MCQCAP register on an UFSHCI 
3.0 controller? -1 or 0?

> +	hba->ext_iid_sup = (hba->mcq_capabilities & MASK_EXT_IID_SUPPORT) >>
> +		EXT_IID_CAP_SHIFT;

[ ... ]

> +	if (dev_info->wspecversion < 0x400)
> +		goto out;

Isn't this version check superfluous? Only UFSHCI 4.0 controllers should 
support the extended IID feature.

> +	ext_ufs_feature = get_unaligned_be32(desc_buf + DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP);

The above change introduces a third instance of this code. Please 
introduce a helper function that does something like the following and 
replace the above line with a call to that helper function:

	if (hba->desc_size[QUERY_DESC_IDN_DEVICE] <
	    DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 4)
		return 0;

	return get_unaligned_be32(desc_buf +
				 DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP);

> +out:
> +	dev_info->b_ext_iid_en = !!ext_iid_en;

Please remove "!!". This conversion happens implicitly when assigning to 
a boolean variable.

Thanks,

Bart.