[PATCH v8 0/3] ahci_xgene: Fixes related to APM X-Gene SATA host controller driver.

[PATCH v8 0/3] ahci_xgene: Fixes related to APM X-Gene SATA host controller driver.

[Suman Tripathi]

This patch set contains a couple of fixes related to APM X-Gene SATA
controller driver.

v2 Change:
   1. Drop the Link down retry patch from this patch set.

v4 Change:
   1. Drop the patch to fix the csr-mask in dts for PHY clock
     node of SATA Host Controller 1.
   2. Add the patch to correct the OOB tunning parameters for
     the COMINIT/COMWAKE parameters.
   3. Add the patch to remove the NCQ support from the APM
     X-Gene AHCI SATA Host controller driver.
   4. Add the patch to remove the clock and PHY reference nodes
     from the APM X-Gene Host controller dts node.

v5 Change :
   1. All the patches are based on 3.16.0-rc6/for-3.17 kernel.
   2. Drop the patch to remove the clock and PHY reference nodes
     from the APM X-Gene Host controller dts node as it breaks
     with old firmware.
   3. Add the patch to skip phy and clock initialisation if
     already done in the firmware.
   4. Add the patch to fix the csr-mask in dts for PHY clock
     node of SATA Host Controller 1.
   5. Add the patch to remove the NCQ support from the APM
     X-Gene AHCI SATA Host controller driver based on 3.16.0-rc6/
     for-3.17 kernel.
   6. Drop the patch to correct the OOB tunning parameters for
     the COMINIT/COMWAKE parameters as it is already applied to
     3.16/for-3.17 branch by the maintainer.
   7. Drop the patch to fix the watermark threshold as it is
     already applied to 3.16/for-3.17 branch by the maintainer.

v6 change :
   1. Drop the patch to skip phy and clock initialisation if
     already done in the firmware.
   2. Drop the patch to fix the csr-mask in dts for PHY clock
     node of SATA Host Controller 1.
   3. Add the remove the clock and PHY node patch as it
     fixes the dropped patches together. This patch works with
     old and new firmware as well.

v7 change :
   1. Drop the patch to remove the clock and PHY reference nodes
     from the APM X-Gene Host controller dts node as it breaks
     with old firmware if sata not initialised from the firmware.
   2. Add the patch to skip phy and clock initialisation if
     already done in the firmware.
   3. Add the patch to fix the csr-mask in dts for PHY clock
     node of SATA Host Controller 1.
   4. Add the Link down retry patch with a proper comments and
     explanation.
   5. Drop the patch to remove NCQ as it is applied to 3.16/for-3.17.

v8 change :
   1. Adding proper comments for link down retry patch.
   2. changing the return type to bool for skip phy patch.

Signed-off-by: Loc Ho <lho@apm.com>
Signed-off-by: Suman Tripathi <stripathi@apm.com>
---

Suman Tripathi (3):
  arm64: Fix the csr-mask for APM X-Gene SoC AHCI SATA PHY clock DTS
    node.
  ahci_xgene: Skip the PHY and clock initialization if already
    configured by the firmware.
  ahci_xgene: Fix the link down in first attempt for the APM X-Gene SoC
    AHCI SATA host controller driver.

 arch/arm64/boot/dts/apm-storm.dtsi |  5 +----
 drivers/ata/ahci_xgene.c           | 41 +++++++++++++++++++++++++++++++-------
 2 files changed, 35 insertions(+), 11 deletions(-)

--
1.8.2.1

[PATCH v8 1/3] arm64: Fix the csr-mask for APM X-Gene SoC AHCI SATA PHY clock DTS node.

[Suman Tripathi]

The value of the csr-mask of the SATA PHY clock DTS node has a
wrong value resulting a kernel panic as the clock/reset is not
proper for the PHY of the SATA host controller 1. This patch
fixes the correct csr-mask value of the SATA PHY clock DTS node
for the SATA Host controller 1.

As the 'ok' is the default status of a device tree node, this patch
removes the status of the PHY clock node of SATA Host Controller 1.
The status of the clock node is handled from the firmware based on
the controller enabled/disabled by the user.

Signed-off-by: Loc Ho <lho@apm.com>
Signed-off-by: Suman Tripathi <stripathi@apm.com>
---
 arch/arm64/boot/dts/apm-storm.dtsi | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi
index 40aa96c..1bdaeda 100644
--- a/arch/arm64/boot/dts/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm-storm.dtsi
@@ -184,9 +184,8 @@
 				reg = <0x0 0x1f21c000 0x0 0x1000>;
 				reg-names = "csr-reg";
 				clock-output-names = "sataphy1clk";
-				status = "disabled";
 				csr-offset = <0x4>;
-				csr-mask = <0x00>;
+				csr-mask = <0x3a>;
 				enable-offset = <0x0>;
 				enable-mask = <0x06>;
 			};
@@ -198,7 +197,6 @@
 				reg = <0x0 0x1f22c000 0x0 0x1000>;
 				reg-names = "csr-reg";
 				clock-output-names = "sataphy2clk";
-				status = "ok";
 				csr-offset = <0x4>;
 				csr-mask = <0x3a>;
 				enable-offset = <0x0>;
@@ -212,7 +210,6 @@
 				reg = <0x0 0x1f23c000 0x0 0x1000>;
 				reg-names = "csr-reg";
 				clock-output-names = "sataphy3clk";
-				status = "ok";
 				csr-offset = <0x4>;
 				csr-mask = <0x3a>;
 				enable-offset = <0x0>;
--
1.8.2.1

[PATCH v8 2/3] ahci_xgene: Skip the PHY and clock initialization if already configured by the firmware.

[Suman Tripathi]

This patch implements the feature to skip the PHY and clock
initialization if it is already configured by the firmware.

Signed-off-by: Loc Ho <lho@apm.com>
Signed-off-by: Suman Tripathi <stripathi@apm.com>
---
 drivers/ata/ahci_xgene.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/ata/ahci_xgene.c b/drivers/ata/ahci_xgene.c
index f416495..fd9c137 100644
--- a/drivers/ata/ahci_xgene.c
+++ b/drivers/ata/ahci_xgene.c
@@ -145,6 +145,15 @@ static unsigned int xgene_ahci_qc_issue(struct ata_queued_cmd *qc)
 	return rc;
 }

+static bool xgene_ahci_is_memram_inited(struct xgene_ahci_context *ctx)
+{
+	void __iomem *diagcsr = ctx->csr_diag;
+
+	return (readl(diagcsr + CFG_MEM_RAM_SHUTDOWN) == 0 &&
+	        readl(diagcsr + BLOCK_MEM_RDY) == 0xFFFFFFFF) ?
+		true : false;
+}
+
 /**
  * xgene_ahci_read_id - Read ID data from the specified device
  * @dev: device
@@ -468,6 +477,11 @@ static int xgene_ahci_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}

+	if (xgene_ahci_is_memram_inited(ctx)) {
+		dev_info(dev, "skip clock and PHY initialization\n");
+		goto skip_clk_phy;
+	}
+
 	/* Due to errata, HW requires full toggle transition */
 	rc = ahci_platform_enable_clks(hpriv);
 	if (rc)
@@ -481,6 +495,8 @@ static int xgene_ahci_probe(struct platform_device *pdev)
 	/* Configure the host controller */
 	xgene_ahci_hw_init(hpriv);

+skip_clk_phy:
+
 	hflags = AHCI_HFLAG_NO_PMP | AHCI_HFLAG_NO_NCQ;

 	rc = ahci_platform_init_host(pdev, hpriv, &xgene_ahci_port_info,
--
1.8.2.1

[PATCH v8 3/3] ahci_xgene: Fix the link down in first attempt for the APM X-Gene SoC AHCI SATA host controller driver.

[Suman Tripathi]

This patch addresses two HW erratas as described below by retrying the
COMRESET:

1. During speed negotiation, controller is not able to detect ALIGN
at GEN3(6Gbps) within 54.6us and results in a timeout. This issue can
be recovered by issuing a COMRESET.

2. Although ALIGN detection is successful, 8b/10b and disparity bit
errors can occur which result in the signature FIS not received
successfully by the Host controller. Due to this, the PHY communication
between the host and drive is not established because of CDR(clock and
data recovery) circuit doesn't lock. This issue can be recoverd by issuing
a COMRESET.

The above retries are issued only if the port status register PXSTATUS
reports device presence detected but PHY communication not established.
The maximum retry attempts are 3.
---
 drivers/ata/ahci_xgene.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/ata/ahci_xgene.c b/drivers/ata/ahci_xgene.c
index fd9c137..894fa33 100644
--- a/drivers/ata/ahci_xgene.c
+++ b/drivers/ata/ahci_xgene.c
@@ -78,6 +78,9 @@
 #define CFG_MEM_RAM_SHUTDOWN		0x00000070
 #define BLOCK_MEM_RDY			0x00000074

+/* Max retry for link down */
+#define MAX_LINK_DOWN_RETRY 3
+
 struct xgene_ahci_context {
 	struct ahci_host_priv *hpriv;
 	struct device *dev;
@@ -276,15 +279,23 @@ static int xgene_ahci_do_hardreset(struct ata_link *link,
 	u8 *d2h_fis = pp->rx_fis + RX_FIS_D2H_REG;
 	void __iomem *port_mmio = ahci_port_base(ap);
 	struct ata_taskfile tf;
+	int link_down_retry = 0;
 	int rc;
-	u32 val;
-
-	/* clear D2H reception area to properly wait for D2H FIS */
-	ata_tf_init(link->device, &tf);
-	tf.command = ATA_BUSY;
-	ata_tf_to_fis(&tf, 0, 0, d2h_fis);
-	rc = sata_link_hardreset(link, timing, deadline, online,
+	u32 val, sstatus;
+
+	do {
+		/* clear D2H reception area to properly wait for D2H FIS */
+		ata_tf_init(link->device, &tf);
+		tf.command = ATA_BUSY;
+		ata_tf_to_fis(&tf, 0, 0, d2h_fis);
+		rc = sata_link_hardreset(link, timing, deadline, online,
 				 ahci_check_ready);
+		if (*online)
+			break;
+
+		sata_scr_read(link, SCR_STATUS, &sstatus);
+	} while (link_down_retry++ < MAX_LINK_DOWN_RETRY &&
+		 (sstatus & 0xff) == 0x1);

 	val = readl(port_mmio + PORT_SCR_ERR);
 	if (val & (SERR_DISPARITY | SERR_10B_8B_ERR))
--
1.8.2.1

Re: [PATCH v8 2/3] ahci_xgene: Skip the PHY and clock initialization if already configured by the firmware.

[Sergei Shtylyov]

Hello.

On 08/23/2014 10:37 PM, Suman Tripathi wrote:

> This patch implements the feature to skip the PHY and clock
> initialization if it is already configured by the firmware.

> Signed-off-by: Loc Ho <lho@apm.com>
> Signed-off-by: Suman Tripathi <stripathi@apm.com>
> ---
>   drivers/ata/ahci_xgene.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)

> diff --git a/drivers/ata/ahci_xgene.c b/drivers/ata/ahci_xgene.c
> index f416495..fd9c137 100644
> --- a/drivers/ata/ahci_xgene.c
> +++ b/drivers/ata/ahci_xgene.c
> @@ -145,6 +145,15 @@ static unsigned int xgene_ahci_qc_issue(struct ata_queued_cmd *qc)
>   	return rc;
>   }
>
> +static bool xgene_ahci_is_memram_inited(struct xgene_ahci_context *ctx)
> +{
> +	void __iomem *diagcsr = ctx->csr_diag;
> +
> +	return (readl(diagcsr + CFG_MEM_RAM_SHUTDOWN) == 0 &&
> +	        readl(diagcsr + BLOCK_MEM_RDY) == 0xFFFFFFFF) ?
> +		true : false;

    Just return the result of &&, that should be enough.

WBR, Sergei

[PATCH v8 2/3] ahci_xgene: Skip the PHY and clock initialization if already configured by the firmware.

[Sergei Shtylyov]

Hello.

On 08/23/2014 10:37 PM, Suman Tripathi wrote:

> This patch implements the feature to skip the PHY and clock
> initialization if it is already configured by the firmware.

> Signed-off-by: Loc Ho <lho@apm.com>
> Signed-off-by: Suman Tripathi <stripathi@apm.com>
> ---
>   drivers/ata/ahci_xgene.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)

> diff --git a/drivers/ata/ahci_xgene.c b/drivers/ata/ahci_xgene.c
> index f416495..fd9c137 100644
> --- a/drivers/ata/ahci_xgene.c
> +++ b/drivers/ata/ahci_xgene.c
> @@ -145,6 +145,15 @@ static unsigned int xgene_ahci_qc_issue(struct ata_queued_cmd *qc)
>   	return rc;
>   }
>
> +static bool xgene_ahci_is_memram_inited(struct xgene_ahci_context *ctx)
> +{
> +	void __iomem *diagcsr = ctx->csr_diag;
> +
> +	return (readl(diagcsr + CFG_MEM_RAM_SHUTDOWN) == 0 &&
> +	        readl(diagcsr + BLOCK_MEM_RDY) == 0xFFFFFFFF) ?
> +		true : false;

    Just return the result of &&, that should be enough.

WBR, Sergei

Re: [PATCH v8 3/3] ahci_xgene: Fix the link down in first attempt for the APM X-Gene SoC AHCI SATA host controller driver.

[Tejun Heo]

On Sun, Aug 24, 2014 at 12:07:27AM +0530, Suman Tripathi wrote:
> This patch addresses two HW erratas as described below by retrying the
> COMRESET:
> 
> 1. During speed negotiation, controller is not able to detect ALIGN
> at GEN3(6Gbps) within 54.6us and results in a timeout. This issue can
> be recovered by issuing a COMRESET.
> 
> 2. Although ALIGN detection is successful, 8b/10b and disparity bit
> errors can occur which result in the signature FIS not received
> successfully by the Host controller. Due to this, the PHY communication
> between the host and drive is not established because of CDR(clock and
> data recovery) circuit doesn't lock. This issue can be recoverd by issuing
> a COMRESET.
> 
> The above retries are issued only if the port status register PXSTATUS
> reports device presence detected but PHY communication not established.
> The maximum retry attempts are 3.

Didn't I ask you to update the comment to explain what's going on?  Or
is the existing comment already sufficient?

Thanks.

-- 
tejun

[PATCH v8 3/3] ahci_xgene: Fix the link down in first attempt for the APM X-Gene SoC AHCI SATA host controller driver.

[Tejun Heo]

On Sun, Aug 24, 2014 at 12:07:27AM +0530, Suman Tripathi wrote:
> This patch addresses two HW erratas as described below by retrying the
> COMRESET:
> 
> 1. During speed negotiation, controller is not able to detect ALIGN
> at GEN3(6Gbps) within 54.6us and results in a timeout. This issue can
> be recovered by issuing a COMRESET.
> 
> 2. Although ALIGN detection is successful, 8b/10b and disparity bit
> errors can occur which result in the signature FIS not received
> successfully by the Host controller. Due to this, the PHY communication
> between the host and drive is not established because of CDR(clock and
> data recovery) circuit doesn't lock. This issue can be recoverd by issuing
> a COMRESET.
> 
> The above retries are issued only if the port status register PXSTATUS
> reports device presence detected but PHY communication not established.
> The maximum retry attempts are 3.

Didn't I ask you to update the comment to explain what's going on?  Or
is the existing comment already sufficient?

Thanks.

-- 
tejun

Re: [PATCH v8 3/3] ahci_xgene: Fix the link down in first attempt for the APM X-Gene SoC AHCI SATA host controller driver.

[Tejun Heo]

On Tue, Aug 26, 2014 at 12:17:35PM +0530, Suman Tripathi wrote:
> Didn't I ask you to update the comment to explain what's going on?
> [suman] : can you specifically tell which part of the comment is not clear
> and need more explanation?

The comment on top of the function doesn't seem to match what's being
implemented.  In addition, it's generally not very useful to list the
actual algorithm in text.  Put algorithm in code and explain the
summary and rationales for it in the comments.  Nothing explains why
the retries are being done.

> is the existing comment already sufficient?
> [suman] : The existing comment is sufficient .

No, this isn't.  You don't have to include a novel to explain it but
there's something different going on here and you should provide
information on why this sort of deviation is necessary.

-- 
tejun

[PATCH v8 3/3] ahci_xgene: Fix the link down in first attempt for the APM X-Gene SoC AHCI SATA host controller driver.

[Tejun Heo]

On Tue, Aug 26, 2014 at 12:17:35PM +0530, Suman Tripathi wrote:
> Didn't I ask you to update the comment to explain what's going on?
> [suman] : can you specifically tell which part of the comment is not clear
> and need more explanation?

The comment on top of the function doesn't seem to match what's being
implemented.  In addition, it's generally not very useful to list the
actual algorithm in text.  Put algorithm in code and explain the
summary and rationales for it in the comments.  Nothing explains why
the retries are being done.

> is the existing comment already sufficient?
> [suman] : The existing comment is sufficient .

No, this isn't.  You don't have to include a novel to explain it but
there's something different going on here and you should provide
information on why this sort of deviation is necessary.

-- 
tejun