[PATCH 0/3] Enable DMA clustering in the UFS driver

[PATCH 0/3] Enable DMA clustering in the UFS driver

[Bart Van Assche]

Hi Martin,

The third patch in this series enables DMA clustering in the UFS driver since
UFS host controllers support DMA clustering. The first two patches fix bugs in
the Exynos host controller driver.

Please consider this patch series for the next merge window.

Thanks,

Bart.

Bart Van Assche (3):
  scsi: ufs: Exynos: Fix DMA alignment for PAGE_SIZE != 4096
  scsi: ufs: Exynos: Fix the maximum segment size
  scsi: ufs: Enable DMA clustering

 drivers/ufs/core/ufshcd.c     |  5 ++---
 drivers/ufs/host/ufs-exynos.c | 12 +++++++++---
 include/ufs/ufshcd.h          |  4 ++--
 3 files changed, 13 insertions(+), 8 deletions(-)

[PATCH 1/3] scsi: ufs: Exynos: Fix DMA alignment for PAGE_SIZE != 4096

[Bart Van Assche]

The Exynos UFS controller only supports scatter/gather list elements
that are aligned on a 4 KiB boundary. Fix DMA alignment in case
PAGE_SIZE != 4096. Rename UFSHCD_QUIRK_ALIGN_SG_WITH_PAGE_SIZE into
UFSHCD_QUIRK_4KB_DMA_ALIGNMENT.

Cc: Kiwoong Kim <kwmad.kim@samsung.com>
Fixes: 2b2bfc8aa519 ("scsi: ufs: Introduce a quirk to allow only page-aligned sg entries")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c     | 4 ++--
 drivers/ufs/host/ufs-exynos.c | 2 +-
 include/ufs/ufshcd.h          | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 99ca5b035028..be18edf4ef7f 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -5029,8 +5029,8 @@ static int ufshcd_slave_configure(struct scsi_device *sdev)
 	ufshcd_hpb_configure(hba, sdev);
 
 	blk_queue_update_dma_pad(q, PRDT_DATA_BYTE_COUNT_PAD - 1);
-	if (hba->quirks & UFSHCD_QUIRK_ALIGN_SG_WITH_PAGE_SIZE)
-		blk_queue_update_dma_alignment(q, PAGE_SIZE - 1);
+	if (hba->quirks & UFSHCD_QUIRK_4KB_DMA_ALIGNMENT)
+		blk_queue_update_dma_alignment(q, 4096 - 1);
 	/*
 	 * Block runtime-pm until all consumers are added.
 	 * Refer ufshcd_setup_links().
diff --git a/drivers/ufs/host/ufs-exynos.c b/drivers/ufs/host/ufs-exynos.c
index c3628a8645a5..3cdac89a28b8 100644
--- a/drivers/ufs/host/ufs-exynos.c
+++ b/drivers/ufs/host/ufs-exynos.c
@@ -1673,7 +1673,7 @@ static const struct exynos_ufs_drv_data exynos_ufs_drvs = {
 				  UFSHCD_QUIRK_BROKEN_OCS_FATAL_ERROR |
 				  UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL |
 				  UFSHCD_QUIRK_SKIP_DEF_UNIPRO_TIMEOUT_SETTING |
-				  UFSHCD_QUIRK_ALIGN_SG_WITH_PAGE_SIZE,
+				  UFSHCD_QUIRK_4KB_DMA_ALIGNMENT,
 	.opts			= EXYNOS_UFS_OPT_HAS_APB_CLK_CTRL |
 				  EXYNOS_UFS_OPT_BROKEN_AUTO_CLK_CTRL |
 				  EXYNOS_UFS_OPT_BROKEN_RX_SEL_IDX |
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index dd5912b4db77..583611444f12 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -567,9 +567,9 @@ enum ufshcd_quirks {
 	UFSHCD_QUIRK_SKIP_DEF_UNIPRO_TIMEOUT_SETTING = 1 << 13,
 
 	/*
-	 * This quirk allows only sg entries aligned with page size.
+	 * Align DMA SG entries on a 4 KiB boundary.
 	 */
-	UFSHCD_QUIRK_ALIGN_SG_WITH_PAGE_SIZE		= 1 << 14,
+	UFSHCD_QUIRK_4KB_DMA_ALIGNMENT			= 1 << 14,
 
 	/*
 	 * This quirk needs to be enabled if the host controller does not

RE: [PATCH 1/3] scsi: ufs: Exynos: Fix DMA alignment for PAGE_SIZE != 4096

[Alim Akhtar]

Hi Bart

>-----Original Message-----
>From: Bart Van Assche [mailto:bvanassche@acm.org]
>Sent: Saturday, January 7, 2023 3:28 AM
>To: Martin K . Petersen <martin.petersen@oracle.com>
>Cc: Jaegeuk Kim <jaegeuk@kernel.org>; Avri Altman
><avri.altman@wdc.com>; Adrian Hunter <adrian.hunter@intel.com>; linux-
>scsi@vger.kernel.org; Bart Van Assche <bvanassche@acm.org>; Kiwoong Kim
><kwmad.kim@samsung.com>; James E.J. Bottomley <jejb@linux.ibm.com>;
>Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>; Bean Huo
><beanhuo@micron.com>; Stanley Chu <stanley.chu@mediatek.com>;
>Jinyoung Choi <j-young.choi@samsung.com>; Chanho Park
><chanho61.park@samsung.com>; Alim Akhtar <alim.akhtar@samsung.com>;
>Arthur Simchaev <Arthur.Simchaev@wdc.com>; Keoseong Park
><keosung.park@samsung.com>; Yoshihiro Shimoda
><yoshihiro.shimoda.uh@renesas.com>
>Subject: [PATCH 1/3] scsi: ufs: Exynos: Fix DMA alignment for PAGE_SIZE !=
>4096
>
>The Exynos UFS controller only supports scatter/gather list elements that
are
>aligned on a 4 KiB boundary. Fix DMA alignment in case PAGE_SIZE != 4096.
>Rename UFSHCD_QUIRK_ALIGN_SG_WITH_PAGE_SIZE into
>UFSHCD_QUIRK_4KB_DMA_ALIGNMENT.
>
>Cc: Kiwoong Kim <kwmad.kim@samsung.com>
>Fixes: 2b2bfc8aa519 ("scsi: ufs: Introduce a quirk to allow only
page-aligned sg
>entries")
>Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>---
Thanks!

Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>

Tested on platforms containing Exynos HCI, so feel free to add

Tested-by: Alim Akhtar <alim.akhtar@samsung.com>

> drivers/ufs/core/ufshcd.c     | 4 ++--
> drivers/ufs/host/ufs-exynos.c | 2 +-
> include/ufs/ufshcd.h          | 4 ++--
> 3 files changed, 5 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index
>99ca5b035028..be18edf4ef7f 100644
>--- a/drivers/ufs/core/ufshcd.c
>+++ b/drivers/ufs/core/ufshcd.c
>@@ -5029,8 +5029,8 @@ static int ufshcd_slave_configure(struct scsi_device
>*sdev)
> 	ufshcd_hpb_configure(hba, sdev);
>
> 	blk_queue_update_dma_pad(q, PRDT_DATA_BYTE_COUNT_PAD -
>1);
>-	if (hba->quirks & UFSHCD_QUIRK_ALIGN_SG_WITH_PAGE_SIZE)
>-		blk_queue_update_dma_alignment(q, PAGE_SIZE - 1);
>+	if (hba->quirks & UFSHCD_QUIRK_4KB_DMA_ALIGNMENT)
>+		blk_queue_update_dma_alignment(q, 4096 - 1);
> 	/*
> 	 * Block runtime-pm until all consumers are added.
> 	 * Refer ufshcd_setup_links().
>diff --git a/drivers/ufs/host/ufs-exynos.c b/drivers/ufs/host/ufs-exynos.c
>index c3628a8645a5..3cdac89a28b8 100644
>--- a/drivers/ufs/host/ufs-exynos.c
>+++ b/drivers/ufs/host/ufs-exynos.c
>@@ -1673,7 +1673,7 @@ static const struct exynos_ufs_drv_data
>exynos_ufs_drvs = {
>
>UFSHCD_QUIRK_BROKEN_OCS_FATAL_ERROR |
>
>UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL |
>
>UFSHCD_QUIRK_SKIP_DEF_UNIPRO_TIMEOUT_SETTING |
>-
>UFSHCD_QUIRK_ALIGN_SG_WITH_PAGE_SIZE,
>+				  UFSHCD_QUIRK_4KB_DMA_ALIGNMENT,
> 	.opts			= EXYNOS_UFS_OPT_HAS_APB_CLK_CTRL |
>
>EXYNOS_UFS_OPT_BROKEN_AUTO_CLK_CTRL |
> 				  EXYNOS_UFS_OPT_BROKEN_RX_SEL_IDX |
>diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index
>dd5912b4db77..583611444f12 100644
>--- a/include/ufs/ufshcd.h
>+++ b/include/ufs/ufshcd.h
>@@ -567,9 +567,9 @@ enum ufshcd_quirks {
> 	UFSHCD_QUIRK_SKIP_DEF_UNIPRO_TIMEOUT_SETTING = 1 << 13,
>
> 	/*
>-	 * This quirk allows only sg entries aligned with page size.
>+	 * Align DMA SG entries on a 4 KiB boundary.
> 	 */
>-	UFSHCD_QUIRK_ALIGN_SG_WITH_PAGE_SIZE		= 1 << 14,
>+	UFSHCD_QUIRK_4KB_DMA_ALIGNMENT			= 1 <<
>14,
>
> 	/*
> 	 * This quirk needs to be enabled if the host controller does not

[PATCH 2/3] scsi: ufs: Exynos: Fix the maximum segment size

[Bart Van Assche]

Prepare for enabling DMA clustering and also for supporting
PAGE_SIZE != 4096 by declaring explicitly that the maximum segment
size is 4096 bytes for Exynos UFS host controllers.

Cc: Kiwoong Kim <kwmad.kim@samsung.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/host/ufs-exynos.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/ufs/host/ufs-exynos.c b/drivers/ufs/host/ufs-exynos.c
index 3cdac89a28b8..821c000ca6b0 100644
--- a/drivers/ufs/host/ufs-exynos.c
+++ b/drivers/ufs/host/ufs-exynos.c
@@ -1586,15 +1586,21 @@ static int exynos_ufs_probe(struct platform_device *pdev)
 	const struct ufs_hba_variant_ops *vops = &ufs_hba_exynos_ops;
 	const struct exynos_ufs_drv_data *drv_data =
 		device_get_match_data(dev);
+	struct ufs_hba *hba;
 
 	if (drv_data && drv_data->vops)
 		vops = drv_data->vops;
 
 	err = ufshcd_pltfrm_init(pdev, vops);
-	if (err)
+	if (err) {
 		dev_err(dev, "ufshcd_pltfrm_init() failed %d\n", err);
+		return err;
+	}
+
+	hba = dev_get_drvdata(dev);
+	hba->host->max_segment_size = 4096;
 
-	return err;
+	return 0;
 }
 
 static int exynos_ufs_remove(struct platform_device *pdev)

RE: [PATCH 2/3] scsi: ufs: Exynos: Fix the maximum segment size

[Alim Akhtar]

Hi Bart

>-----Original Message-----
>From: Bart Van Assche [mailto:bvanassche@acm.org]
>Sent: Saturday, January 7, 2023 3:28 AM
>To: Martin K . Petersen <martin.petersen@oracle.com>
>Cc: Jaegeuk Kim <jaegeuk@kernel.org>; Avri Altman
><avri.altman@wdc.com>; Adrian Hunter <adrian.hunter@intel.com>; linux-
>scsi@vger.kernel.org; Bart Van Assche <bvanassche@acm.org>; Kiwoong Kim
><kwmad.kim@samsung.com>; James E.J. Bottomley <jejb@linux.ibm.com>;
>Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>; Chanho Park
><chanho61.park@samsung.com>; Alim Akhtar <alim.akhtar@samsung.com>;
>Bean Huo <beanhuo@micron.com>
>Subject: [PATCH 2/3] scsi: ufs: Exynos: Fix the maximum segment size
>
>Prepare for enabling DMA clustering and also for supporting PAGE_SIZE !=
>4096 by declaring explicitly that the maximum segment size is 4096 bytes
for
>Exynos UFS host controllers.
>
>Cc: Kiwoong Kim <kwmad.kim@samsung.com>
>Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>---

Thanks!

Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>

Tested on platforms containing Exynos HCI, so feel free to add

Tested-by: Alim Akhtar <alim.akhtar@samsung.com>


For some reason, I didn't receive you patch-3/3 in my inbox.

> drivers/ufs/host/ufs-exynos.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/ufs/host/ufs-exynos.c b/drivers/ufs/host/ufs-exynos.c
>index 3cdac89a28b8..821c000ca6b0 100644
>--- a/drivers/ufs/host/ufs-exynos.c
>+++ b/drivers/ufs/host/ufs-exynos.c
>@@ -1586,15 +1586,21 @@ static int exynos_ufs_probe(struct
>platform_device *pdev)
> 	const struct ufs_hba_variant_ops *vops = &ufs_hba_exynos_ops;
> 	const struct exynos_ufs_drv_data *drv_data =
> 		device_get_match_data(dev);
>+	struct ufs_hba *hba;
>
> 	if (drv_data && drv_data->vops)
> 		vops = drv_data->vops;
>
> 	err = ufshcd_pltfrm_init(pdev, vops);
>-	if (err)
>+	if (err) {
> 		dev_err(dev, "ufshcd_pltfrm_init() failed %d\n", err);
>+		return err;
>+	}
>+
>+	hba = dev_get_drvdata(dev);
>+	hba->host->max_segment_size = 4096;
>
>-	return err;
>+	return 0;
> }
>
> static int exynos_ufs_remove(struct platform_device *pdev)

Re: [PATCH 2/3] scsi: ufs: Exynos: Fix the maximum segment size

[Bart Van Assche]

On 1/10/23 22:21, Alim Akhtar wrote:
> For some reason, I didn't receive you patch-3/3 in my inbox.

That's unfortunate. That's probably because I use a shell script to 
generate the Cc-list for UFS driver patches:

scripts/get_maintainer.pl --no-git --no-l --no-r --no-roles 
--no-rolestats "${1:-&STDIN}"

Anyway, patch 3/3 is available here: 
https://lore.kernel.org/linux-scsi/20230106215800.2249344-4-bvanassche@acm.org/

Bart.

[PATCH 3/3] scsi: ufs: Enable DMA clustering

[Bart Van Assche]

All UFS host controllers support DMA clustering. Hence enable DMA
clustering.

Note: without patch "Exynos: Fix the maximum segment size", this patch
breaks support for the Exynos controller.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index be18edf4ef7f..fe83fdda8d23 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8429,7 +8429,6 @@ static struct scsi_host_template ufshcd_driver_template = {
 	.max_host_blocked	= 1,
 	.track_queue_depth	= 1,
 	.sdev_groups		= ufshcd_driver_groups,
-	.dma_boundary		= PAGE_SIZE - 1,
 	.rpm_autosuspend_delay	= RPM_AUTOSUSPEND_DELAY_MS,
 };
 

RE: [PATCH 3/3] scsi: ufs: Enable DMA clustering

[Avri Altman]

> All UFS host controllers support DMA clustering. Hence enable DMA clustering.
> 
> Note: without patch "Exynos: Fix the maximum segment size", this patch breaks
> support for the Exynos controller.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ufs/core/ufshcd.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index
> be18edf4ef7f..fe83fdda8d23 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8429,7 +8429,6 @@ static struct scsi_host_template
> ufshcd_driver_template = {
>         .max_host_blocked       = 1,
>         .track_queue_depth      = 1,
>         .sdev_groups            = ufshcd_driver_groups,
> -       .dma_boundary           = PAGE_SIZE - 1,
Isn't DBC <= 256KB implies that for a boundary?

Thanks,
Avri

>         .rpm_autosuspend_delay  = RPM_AUTOSUSPEND_DELAY_MS,  };

Re: [PATCH 3/3] scsi: ufs: Enable DMA clustering

[Bart Van Assche]

On 1/8/23 00:20, Avri Altman wrote:
>> All UFS host controllers support DMA clustering. Hence enable DMA clustering.
>>
>> Note: without patch "Exynos: Fix the maximum segment size", this patch breaks
>> support for the Exynos controller.
>>
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> ---
>>   drivers/ufs/core/ufshcd.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index
>> be18edf4ef7f..fe83fdda8d23 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -8429,7 +8429,6 @@ static struct scsi_host_template
>> ufshcd_driver_template = {
>>          .max_host_blocked       = 1,
>>          .track_queue_depth      = 1,
>>          .sdev_groups            = ufshcd_driver_groups,
>> -       .dma_boundary           = PAGE_SIZE - 1,
 >
> Isn't DBC <= 256KB implies that for a boundary?

Hi Avri,

I don't think so. I think that the 256 KiB limit for the PRDT 
corresponds to the max_segment_size parameter. The dma_boundary 
parameter represents a boundary that must not be crossed by DMA 
scatter/gather lists. I'm not aware of any restrictions on DMA 
scatter/gather lists for UFS controllers other than the 256 KiB limit 
for a single PRDT and a limitation to 32-bits addresses for controllers 
that only support 32-bits DMA. The latter restriction is already handled 
by ufshcd_set_dma_mask().

Bart.

Re: [PATCH 3/3] scsi: ufs: Enable DMA clustering

[Adrian Hunter]

On 6/01/23 23:58, Bart Van Assche wrote:
> All UFS host controllers support DMA clustering. Hence enable DMA
> clustering.

The patch history of ufshcd.c seems to suggest the dma_boundary
setting was never intentional, but was inherited by default.

However, I guess it is not impossible that removing the setting
exposes issues for existing controllers.

I suggest perhaps expanding upon the commit message and the
cc list so more people will be aware of the change.

Possibly worth including the explanation in your reply to
Avri concerning PRDT 256K DBC limit

> 
> Note: without patch "Exynos: Fix the maximum segment size", this patch
> breaks support for the Exynos controller.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Otherwise:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/ufs/core/ufshcd.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index be18edf4ef7f..fe83fdda8d23 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8429,7 +8429,6 @@ static struct scsi_host_template ufshcd_driver_template = {
>  	.max_host_blocked	= 1,
>  	.track_queue_depth	= 1,
>  	.sdev_groups		= ufshcd_driver_groups,
> -	.dma_boundary		= PAGE_SIZE - 1,
>  	.rpm_autosuspend_delay	= RPM_AUTOSUSPEND_DELAY_MS,
>  };
>  

Re: [PATCH 3/3] scsi: ufs: Enable DMA clustering

[Bart Van Assche]

On 1/9/23 23:56, Adrian Hunter wrote:
> On 6/01/23 23:58, Bart Van Assche wrote:
>> All UFS host controllers support DMA clustering. Hence enable DMA
>> clustering.
> 
> The patch history of ufshcd.c seems to suggest the dma_boundary
> setting was never intentional, but was inherited by default.
> 
> However, I guess it is not impossible that removing the setting
> exposes issues for existing controllers.
> 
> I suggest perhaps expanding upon the commit message and the
> cc list so more people will be aware of the change.
> 
> Possibly worth including the explanation in your reply to
> Avri concerning PRDT 256K DBC limit
> 
>>
>> Note: without patch "Exynos: Fix the maximum segment size", this patch
>> breaks support for the Exynos controller.
>>
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> 
> Otherwise:
> 
> Acked-by: Adrian Hunter <adrian.hunter@intel.com>

Hi Adrian,

I will make the requested changes.

Thanks for the review!

Bart.