Change in coreboot[master]: sc7180: Fix for hang during DMA transfer in SPI-NOR flash driver

[Ravi kumar (Code Review)]

Hello Satya Priya Kakitapalli,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/39807

to review the following change.

Change subject: sc7180: Fix for hang during DMA transfer in SPI-NOR flash driver
......................................................................

sc7180: Fix for hang during DMA transfer in SPI-NOR flash driver

Transfer sequence used by SPI-Flash application present in CB/DC.
1. Assert CS through GPIO
2. Data transfer through QSPI (involves construction of command
   descriptor for multiple read/write transfers)
3. De-assert CS through GPIO.

With above sequence, in DMA mode we dont have the support for read
transfers that are not preceded by write transfer in QSPI controller.
Ex: "write read read read" sequence results in hang during DMA transfer,
where as "write read write read" sequence has no issue.

As we have application controlling CS through GPIO, we are making
fragment bit "set" for all transfers, which keeps CS in asserted
state although the ideal way to operate CS is through QSPI controller.

Change-Id: Ia45ab793ad05861b88e99a320b1ee9f10707def7
Signed-off-by: satya priya 
---
M src/soc/qualcomm/sc7180/qspi.c
1 file changed, 8 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/39807/1

diff --git a/src/soc/qualcomm/sc7180/qspi.c b/src/soc/qualcomm/sc7180/qspi.c
index 30dc1c3..8ecc07f 100644
--- a/src/soc/qualcomm/sc7180/qspi.c
+++ b/src/soc/qualcomm/sc7180/qspi.c
@@ -1,7 +1,7 @@
 /*
  * This file is part of the coreboot project.
  *
- * Copyright (C) 2019, The Linux Foundation.  All rights reserved.
+ * Copyright (C) 2019-2020, The Linux Foundation.  All rights reserved.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 and
@@ -131,17 +131,20 @@
    next->direction = MASTER_READ;
    next->multi_io_mode = 0;
    next->reserved1 = 0;
-	next->fragment = 0;
+	/*
+	 * QSPI controller doesn't support transfer starts with read segment.
+	 * So to support read transfers that are not preceded by write, set
+	 * transfer fragment bit = 1
+	 */
+	next->fragment = 1;
    next->reserved2 = 0;
    next->length = 0;
    next->bounce_src = 0;
    next->bounce_dst = 0;
    next->bounce_length = 0;

-	if (current) {
+	if (current)
    	current->next_descriptor = (uint32_t)(uintptr_t) next;
-		current->fragment = 1;
-	}

return next;
 }

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39807 )

Change subject: sc7180: Fix for hang during DMA transfer in SPI-NOR flash driver
......................................................................

Patch Set 1: Code-Review+2

[mturney mturney (Code Review)]

mturney mturney has uploaded a new patch set (#4) to the change originally created by Ravi kumar. ( https://review.coreboot.org/c/coreboot/+/39807 )

Change subject: sc7180: Fix for hang during DMA transfer in SPI-NOR flash driver
......................................................................

sc7180: Fix for hang during DMA transfer in SPI-NOR flash driver

Transfer sequence used by SPI-Flash application present in CB/DC.
1. Assert CS through GPIO
2. Data transfer through QSPI (involves construction of command
   descriptor for multiple read/write transfers)
3. De-assert CS through GPIO.

With above sequence, in DMA mode we dont have the support for read
transfers that are not preceded by write transfer in QSPI controller.
Ex: "write read read read" sequence results in hang during DMA transfer,
where as "write read write read" sequence has no issue.

As we have application controlling CS through GPIO, we are making
fragment bit "set" for all transfers, which keeps CS in asserted
state although the ideal way to operate CS is through QSPI controller.

Change-Id: Ia45ab793ad05861b88e99a320b1ee9f10707def7
Signed-off-by: satya priya 
---
M src/soc/qualcomm/sc7180/qspi.c
1 file changed, 8 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/39807/4

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39807 )

Change subject: sc7180: Fix for hang during DMA transfer in SPI-NOR flash driver
......................................................................

Patch Set 5: Code-Review+2

Please rebase this in front of the display stuff so that we can merge it while that's still in review.

[mturney mturney (Code Review)]

mturney mturney has uploaded a new patch set (#7) to the change originally created by Ravi kumar. ( https://review.coreboot.org/c/coreboot/+/39807 )

Change subject: sc7180: Fix for hang during DMA transfer in SPI-NOR flash driver
......................................................................

sc7180: Fix for hang during DMA transfer in SPI-NOR flash driver

Transfer sequence used by SPI-Flash application present in CB/DC.
1. Assert CS through GPIO
2. Data transfer through QSPI (involves construction of command
   descriptor for multiple read/write transfers)
3. De-assert CS through GPIO.

With above sequence, in DMA mode we dont have the support for read
transfers that are not preceded by write transfer in QSPI controller.
Ex: "write read read read" sequence results in hang during DMA transfer,
where as "write read write read" sequence has no issue.

As we have application controlling CS through GPIO, we are making
fragment bit "set" for all transfers, which keeps CS in asserted
state although the ideal way to operate CS is through QSPI controller.

Change-Id: Ia45ab793ad05861b88e99a320b1ee9f10707def7
Signed-off-by: satya priya 
---
M src/soc/qualcomm/sc7180/qspi.c
1 file changed, 8 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/39807/7

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39807 )

Change subject: sc7180: Fix for hang during DMA transfer in SPI-NOR flash driver
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39807/7/src/soc/qualcomm/sc7180/qsp... 
File src/soc/qualcomm/sc7180/qspi.c:

https://review.coreboot.org/c/coreboot/+/39807/7/src/soc/qualcomm/sc7180/qsp... 
PS7, Line 5: *
           :  * This program is free software; you can redistribute it and/or modify
           :  * it under the terms of the GNU General Public License version 2 and
           :  * only version 2 as published by the Free Software Foundation.
           :  *
           :  * This program is distributed in the hope that it will be useful,
           :  * but WITHOUT ANY WARRANTY; without even the implied warranty of
           :  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
           :  * GNU General Public License for more details.
           :  */
/* SPDX-License-Identifier: GPL-2.0-only */

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39807 )

Change subject: sc7180: Fix for hang during DMA transfer in SPI-NOR flash driver
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39807/7/src/soc/qualcomm/sc7180/qsp... 
File src/soc/qualcomm/sc7180/qspi.c:

https://review.coreboot.org/c/coreboot/+/39807/7/src/soc/qualcomm/sc7180/qsp... 
PS7, Line 5: *
           :  * This program is free software; you can redistribute it and/or modify
           :  * it under the terms of the GNU General Public License version 2 and
           :  * only version 2 as published by the Free Software Foundation.
           :  *
           :  * This program is distributed in the hope that it will be useful,
           :  * but WITHOUT ANY WARRANTY; without even the implied warranty of
           :  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
           :  * GNU General Public License for more details.
           :  */
> /* SPDX-License-Identifier: GPL-2. […]

That will be taken care of already. The more important part: we don't have copyright lines in individual files anymore, so remove line 4

[mturney mturney (Code Review)]

mturney mturney has uploaded a new patch set (#8) to the change originally created by Ravi kumar. ( https://review.coreboot.org/c/coreboot/+/39807 )

Change subject: sc7180: Fix for hang during DMA transfer in SPI-NOR flash driver
......................................................................

sc7180: Fix for hang during DMA transfer in SPI-NOR flash driver

Transfer sequence used by SPI-Flash application present in CB/DC.
1. Assert CS through GPIO
2. Data transfer through QSPI (involves construction of command
   descriptor for multiple read/write transfers)
3. De-assert CS through GPIO.

With above sequence, in DMA mode we dont have the support for read
transfers that are not preceded by write transfer in QSPI controller.
Ex: "write read read read" sequence results in hang during DMA transfer,
where as "write read write read" sequence has no issue.

As we have application controlling CS through GPIO, we are making
fragment bit "set" for all transfers, which keeps CS in asserted
state although the ideal way to operate CS is through QSPI controller.

Change-Id: Ia45ab793ad05861b88e99a320b1ee9f10707def7
Signed-off-by: satya priya 
---
M src/soc/qualcomm/sc7180/qspi.c
1 file changed, 7 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/39807/8

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39807 )

Change subject: sc7180: Fix for hang during DMA transfer in SPI-NOR flash driver
......................................................................

Patch Set 8: Code-Review+2

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39807 )

Change subject: sc7180: Fix for hang during DMA transfer in SPI-NOR flash driver
......................................................................

Patch Set 8:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39807/7/src/soc/qualcomm/sc7180/qsp... 
File src/soc/qualcomm/sc7180/qspi.c:

https://review.coreboot.org/c/coreboot/+/39807/7/src/soc/qualcomm/sc7180/qsp... 
PS7, Line 5: *
           :  * This program is free software; you can redistribute it and/or modify
           :  * it under the terms of the GNU General Public License version 2 and
           :  * only version 2 as published by the Free Software Foundation.
           :  *
           :  * This program is distributed in the hope that it will be useful,
           :  * but WITHOUT ANY WARRANTY; without even the implied warranty of
           :  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
           :  * GNU General Public License for more details.
           :  */
> That will be taken care of already. […]

Done

[Julius Werner (Code Review)]

Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39807 )

Change subject: sc7180: Fix for hang during DMA transfer in SPI-NOR flash driver
......................................................................

sc7180: Fix for hang during DMA transfer in SPI-NOR flash driver

Transfer sequence used by SPI-Flash application present in CB/DC.
1. Assert CS through GPIO
2. Data transfer through QSPI (involves construction of command
   descriptor for multiple read/write transfers)
3. De-assert CS through GPIO.

With above sequence, in DMA mode we dont have the support for read
transfers that are not preceded by write transfer in QSPI controller.
Ex: "write read read read" sequence results in hang during DMA transfer,
where as "write read write read" sequence has no issue.

As we have application controlling CS through GPIO, we are making
fragment bit "set" for all transfers, which keeps CS in asserted
state although the ideal way to operate CS is through QSPI controller.

Change-Id: Ia45ab793ad05861b88e99a320b1ee9f10707def7
Signed-off-by: satya priya 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/39807
Reviewed-by: Julius Werner 
Tested-by: build bot (Jenkins) 
---
M src/soc/qualcomm/sc7180/qspi.c
1 file changed, 7 insertions(+), 4 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Julius Werner: Looks good to me, approved

diff --git a/src/soc/qualcomm/sc7180/qspi.c b/src/soc/qualcomm/sc7180/qspi.c
index 10038e4..675641b 100644
--- a/src/soc/qualcomm/sc7180/qspi.c
+++ b/src/soc/qualcomm/sc7180/qspi.c
@@ -118,17 +118,20 @@
    next->direction = MASTER_READ;
    next->multi_io_mode = 0;
    next->reserved1 = 0;
-	next->fragment = 0;
+	/*
+	 * QSPI controller doesn't support transfer starts with read segment.
+	 * So to support read transfers that are not preceded by write, set
+	 * transfer fragment bit = 1
+	 */
+	next->fragment = 1;
    next->reserved2 = 0;
    next->length = 0;
    next->bounce_src = 0;
    next->bounce_dst = 0;
    next->bounce_length = 0;

-	if (current) {
+	if (current)
    	current->next_descriptor = (uint32_t)(uintptr_t) next;
-		current->fragment = 1;
-	}

return next;
 }