ahci_xgene: Fixes related to APM X-Gene SATA host controller driver.

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.

---

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

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.

---
 arch/arm64/boot/dts/apm-storm.dtsi | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

index 40aa96c..1bdaeda 100644
@@ -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

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

---
 drivers/ata/ahci_xgene.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

index f416495..fd9c137 100644
@@ -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

Hello.

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

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(-)

index fd9c137..894fa33 100644
@@ -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

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

Thanks.

tejun

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.

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