ata: ahci_xgene: Enable native NCQ and FBS for second generation of X-Gene SoC.

PATCH Superseded Suman Tripathi 5 participants 12 messages Apr 1, 2015

Subsystems (2)

SCSI libata

lore

Share & tools

lore

Suman Tripathi

author

2015-04-01 14:48 UTC #1 lore raw

PATCH 1/1 ata: ahci_xgene: Add AHCI Support for second generation of APM X-Gene SoC

This patch enables full AHCI feature support for APM X-Gene SoC SATA host host controller. The following errata's are removed:

1. 2a0bdff6b95 ("ahci-xgene: fix the dma state machine lockup for the IDENTIFY DEVICE PIO mode command")
2. 09c32aaa368 ("ahci_xgene: Fix the dma state machine lockup for the ATA_CMD_SMART PIO mode command")
3. 1540035da71 ("ahci_xgene: Implement the xgene_ahci_poll_reg_val to support PMP")
4. a3a84bc7c88 ("ahci_xgene: Implement the workaround to support PMP enumeration and discovery")
5. 1102407bb71 ("ahci_xgene: Fix the DMA state machine lockup for the ATA_CMD_PACKET PIO mode command")
6. 72f79f9e35b ("ahci_xgene: Removing NCQ support from the APM X-Gene SoC AHCI SATA Host Controller driver")

In addition, enable PMP support for second generation APM X-Gene SoC.

Signed-off-by Suman Tripathi <[email protected]>

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

drivers/ata/ahci_xgene.c +20 -1

index 2e8bb60..0be47b0 100644
@@ -28,6 +28,8 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/phy/phy.h>
+#include <asm/cputype.h>
+
 #include "ahci.h"
 
 #define DRV_NAME "xgene-ahci"

@@ -94,6 +96,11 @@ struct xgene_ahci_context {
 	void __iomem *csr_mux;		/* MUX CSR address of IP */
 };
 
+static bool xgene_ahci_version1(void)
+{
+	return MIDR_VARIANT(read_cpuid_id()) == 0 ? true : false;
+}
+
 static int xgene_ahci_init_memram(struct xgene_ahci_context *ctx)
 {
 	dev_dbg(ctx->dev, "Release memory from shutdown\n");

@@ -703,7 +710,19 @@ static int xgene_ahci_probe(struct platform_device *pdev)
 	/* Configure the host controller */
 	xgene_ahci_hw_init(hpriv);
 skip_clk_phy:
-	hpriv->flags = AHCI_HFLAG_NO_PMP | AHCI_HFLAG_NO_NCQ;
+
+	if (xgene_ahci_version1()) {
+		/*
+		 * Override the callbacks due to
+		 * XGENE1 ERRATA's.
+		 */
+		hpriv->flags |= AHCI_HFLAG_NO_NCQ;
+		xgene_ahci_ops.qc_issue = xgene_ahci_qc_issue;
+		xgene_ahci_ops.softreset = xgene_ahci_softreset;
+		xgene_ahci_ops.pmp_softreset = xgene_ahci_pmp_softreset;
+	} else {
+		hpriv->flags |= AHCI_HFLAG_YES_FBS;
+	}
 
 	rc = ahci_platform_init_host(pdev, hpriv, &xgene_ahci_port_info,
 				     &ahci_platform_sht);

--
1.8.2.1

Tejun Heo

Applied

2015-04-01 16:13 UTC #2 lore raw

PATCH 1/1 ata: ahci_xgene: Add AHCI Support for second generation of APM X-Gene SoC

Quoted 16 lines · Suman Tripathi

This patch enables full AHCI feature support for APM X-Gene SoC SATA host host controller. The following errata's are removed:
1. 2a0bdff6b95 ("ahci-xgene: fix the dma state machine lockup for the IDENTIFY DEVICE PIO mode command")
2. 09c32aaa368 ("ahci_xgene: Fix the dma state machine lockup for the ATA_CMD_SMART PIO mode command")
3. 1540035da71 ("ahci_xgene: Implement the xgene_ahci_poll_reg_val to support PMP")
4. a3a84bc7c88 ("ahci_xgene: Implement the workaround to support PMP enumeration and discovery")
5. 1102407bb71 ("ahci_xgene: Fix the DMA state machine lockup for the ATA_CMD_PACKET PIO mode command")
6. 72f79f9e35b ("ahci_xgene: Removing NCQ support from the APM X-Gene SoC AHCI SATA Host Controller driver")

Quoted 3 lines

In addition, enable PMP support for second generation APM X-Gene SoC.
Signed-off-by Suman Tripathi <[email protected]>

Applied to libata/for-4.1 w/ minor edit.

drivers/ata/ahci_xgene.c static int xgene_ahci_probe(struct platform_device *pdev) · line 718

Quoted 5 lines

drivers/ata/ahci_xgene.c +5 -0

+	if (xgene_ahci_version1()) {
+		/*
+		 * Override the callbacks due to
+		 * XGENE1 ERRATA's.
+		 */

The column limit is 80 (more like 75), not 40. I collapsed the above into one line.

Thanks.

Signature · 1 line

tejun

Tejun Heo

2015-04-01 16:31 UTC #3 lore raw

PATCH 1/1 ata: ahci_xgene: Add AHCI Support for second generation of APM X-Gene SoC

Quoted 3 lines · Tejun Heo

Quoted 1 line
Signed-off-by Suman Tripathi <[email protected]>
Applied to libata/for-4.1 w/ minor edit.

Reverted due to build failure from missing asm/cputype.h. Suman, you're the develper and familiar with how the prerequisite patches are flowing where. When sending patches to maintainers, please let them know how to deal with your patches. Having cross-tree dependencies are fine, there are multiple ways to deal with that but please don't just drop patches which will fail build and see what happens. Please help us do our job.

Thanks.

Signature · 1 line

tejun

Suman Tripathi

author

2015-04-01 16:37 UTC #4 lore raw

PATCH 1/1 ata: ahci_xgene: Add AHCI Support for second generation of APM X-Gene SoC

Quoted 3 lines · Tejun Heo

Quoted 1 line
Signed-off-by Suman Tripathi <[email protected]>
Applied to libata/for-4.1 w/ minor edit.

Apologize for that . Really sorry . I will check it.

Thanks.

Quoted 17 lines · Tejun Heo

Quoted 3 lines · Tejun Heo
Quoted 1 line
Signed-off-by Suman Tripathi <[email protected]>
Applied to libata/for-4.1 w/ minor edit.
Reverted due to build failure from missing asm/cputype.h. Suman, you're the develper and familiar with how the prerequisite patches are flowing where. When sending patches to maintainers, please let them know how to deal with your patches. Having cross-tree dependencies are fine, there are multiple ways to deal with that but please don't just drop patches which will fail build and see what happens. Please help us do our job.
Thanks.
--
tejun

Signature · 3 lines

Thanks,
with regards,
Suman Tripathi

Russell King - ARM Linux

2015-04-01 16:39 UTC #5 lore raw

PATCH 1/1 ata: ahci_xgene: Add AHCI Support for second generation of APM X-Gene SoC

In reply to Tejun Heo

Quoted 6 lines · Tejun Heo

Quoted 3 lines · Tejun Heo
Quoted 1 line
Signed-off-by Suman Tripathi <[email protected]>
Applied to libata/for-4.1 w/ minor edit.
Reverted due to build failure from missing asm/cputype.h.

I'm guessing this is some kind of ARM driver, which is why linux-arm-kernel has been Cc'd - though I haven't seen the original patch.

ARM has asm/cputype.h, and has had for a long time, so "missing asm/cputype.h" doesn't make sense as a reason to revert it. Maybe it's trying to be built on x86 when it should be restricted to only ARM?

Dunno without seeing the original patch.

Signature · 2 lines

FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Tejun Heo

2015-04-01 16:47 UTC #6 lore raw

PATCH 1/1 ata: ahci_xgene: Add AHCI Support for second generation of APM X-Gene SoC

Quoted 11 lines · Russell King - ARM Linux

Quoted 6 lines · Tejun Heo
Quoted 3 lines · Tejun Heo
Quoted 1 line
Signed-off-by Suman Tripathi <[email protected]>
Applied to libata/for-4.1 w/ minor edit.
Reverted due to build failure from missing asm/cputype.h.
I'm guessing this is some kind of ARM driver, which is why linux-arm-kernel has been Cc'd - though I haven't seen the original patch.

It's ahci_xgene which seems to be specific to ARM.

Quoted 2 lines

ARM has asm/cputype.h, and has had for a long time, so "missing asm/cputype.h" doesn't make sense as a reason to revert it. Maybe

As it's a new build failure I assumed that was a new file. Apparently not.

Quoted 2 lines

it's trying to be built on x86 when it should be restricted to only ARM?

The build failure is from kbuild and it's on PPC64 w/ COMPILE_TEST. Urgh.. Suman, can you please update the patch to turn off COMPILE_TEST on the driver?

Thanks.

Signature · 1 line

tejun

Catalin Marinas

2015-04-01 17:00 UTC #7 lore raw

PATCH 1/1 ata: ahci_xgene: Add AHCI Support for second generation of APM X-Gene SoC

In reply to Russell King - ARM Linux

Quoted 12 lines · Russell King - ARM Linux

Quoted 6 lines · Tejun Heo
Quoted 3 lines · Tejun Heo
Quoted 1 line
Signed-off-by Suman Tripathi <[email protected]>
Applied to libata/for-4.1 w/ minor edit.
Reverted due to build failure from missing asm/cputype.h.
I'm guessing this is some kind of ARM driver, which is why linux-arm-kernel has been Cc'd - though I haven't seen the original patch.

Quoted 6 lines

ARM has asm/cputype.h, and has had for a long time, so "missing asm/cputype.h" doesn't make sense as a reason to revert it. Maybe it's trying to be built on x86 when it should be restricted to only ARM?
Dunno without seeing the original patch.

First search result leads here:

https://www.mail-archive.com/devicetree at vger.kernel.org/msg67774.html

The driver in general should not be ARM specific, though it runs on an ARMv8 platform. But looking at the patch it has some errata workarounds triggered based on the CPU id (MIDR). That looks dodgy as it doesn't even check the full ID, only the variant part.

But if they don't have any other way of identifying the hardware version (ideally some SoC or device identification), the driver should at least be made dependent on ARM64. Looking at the Kconfig entries, PHY_XGENE depends on (ARM64 || COMPILE_TEST). The COMPILE_TEST part should be dropped (I would still prefer a different way to handle this than checking MIDR but I'm not familiar with the hardware).

Signature · 1 line

Catalin

Mark Rutland

2015-04-01 17:07 UTC #8 lore raw

PATCH 1/1 ata: ahci_xgene: Add AHCI Support for second generation of APM X-Gene SoC

Quoted 29 lines · Catalin Marinas

Quoted 12 lines · Russell King - ARM Linux
Quoted 6 lines · Tejun Heo
Quoted 3 lines · Tejun Heo
Quoted 1 line
Signed-off-by Suman Tripathi <[email protected]>
Applied to libata/for-4.1 w/ minor edit.
Reverted due to build failure from missing asm/cputype.h.
I'm guessing this is some kind of ARM driver, which is why linux-arm-kernel has been Cc'd - though I haven't seen the original patch.
Quoted 6 lines
ARM has asm/cputype.h, and has had for a long time, so "missing asm/cputype.h" doesn't make sense as a reason to revert it. Maybe it's trying to be built on x86 when it should be restricted to only ARM?
Dunno without seeing the original patch.
First search result leads here:
https://www.mail-archive.com/devicetree at vger.kernel.org/msg67774.html
The driver in general should not be ARM specific, though it runs on an ARMv8 platform. But looking at the patch it has some errata workarounds triggered based on the CPU id (MIDR). That looks dodgy as it doesn't even check the full ID, only the variant part.

Quoted 6 lines

But if they don't have any other way of identifying the hardware version (ideally some SoC or device identification), the driver should at least be made dependent on ARM64. Looking at the Kconfig entries, PHY_XGENE depends on (ARM64 || COMPILE_TEST). The COMPILE_TEST part should be dropped (I would still prefer a different way to handle this than checking MIDR but I'm not familiar with the hardware).

We should just add a new compatible string for the new revision of the AHCI controller. Given the errata we're avoiding apply to the AHCI controller and not the CPU, looking at the CPU ID is nonsense.

Mark.

Russell King - ARM Linux

2015-04-01 17:30 UTC #9 lore raw

PATCH 1/1 ata: ahci_xgene: Add AHCI Support for second generation of APM X-Gene SoC

In reply to Catalin Marinas

Quoted 18 lines · Catalin Marinas

Quoted 17 lines · Russell King - ARM Linux
Quoted 6 lines · Tejun Heo
Quoted 3 lines · Tejun Heo
Quoted 1 line
Signed-off-by Suman Tripathi <[email protected]>
Applied to libata/for-4.1 w/ minor edit.
Reverted due to build failure from missing asm/cputype.h.
I'm guessing this is some kind of ARM driver, which is why linux-arm-kernel has been Cc'd - though I haven't seen the original patch.
ARM has asm/cputype.h, and has had for a long time, so "missing asm/cputype.h" doesn't make sense as a reason to revert it. Maybe it's trying to be built on x86 when it should be restricted to only ARM?

Quoted 5 lines

Quoted 1 line
Dunno without seeing the original patch.
First search result leads here:
https://www.mail-archive.com/devicetree at vger.kernel.org/msg67774.html

Thanks.

Quoted 4 lines

The driver in general should not be ARM specific, though it runs on an ARMv8 platform. But looking at the patch it has some errata workarounds triggered based on the CPU id (MIDR). That looks dodgy as it doesn't even check the full ID, only the variant part.

Okay, it's not ARM but an ARM64 driver, so that's your territory, not mine. :)

Signature · 2 lines

FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Catalin Marinas

2015-04-01 18:18 UTC #10 lore raw

PATCH 1/1 ata: ahci_xgene: Add AHCI Support for second generation of APM X-Gene SoC

Quoted 8 lines · Russell King - ARM Linux

Quoted 4 lines · Catalin Marinas
The driver in general should not be ARM specific, though it runs on an ARMv8 platform. But looking at the patch it has some errata workarounds triggered based on the CPU id (MIDR). That looks dodgy as it doesn't even check the full ID, only the variant part.
Okay, it's not ARM but an ARM64 driver, so that's your territory, not mine. :)

Well, feel free to enter this territory ;).

Anyway, what I meant is that such driver should not rely on CPU identification at all, especially since it does not require some specific CPU features but it tries to guess which SoC revision it is and which device (not CPU) bugs have been fixed. Better if they follow Mark R's idea to pass such information in DT for the device.

So maybe the revert is not too bad an idea.

Signature · 1 line

Catalin

Mark Rutland

2015-04-01 18:28 UTC #11 lore raw

PATCH 1/1 ata: ahci_xgene: Add AHCI Support for second generation of APM X-Gene SoC

Quoted 12 lines · Catalin Marinas

Quoted 8 lines · Russell King - ARM Linux
Quoted 4 lines · Catalin Marinas
The driver in general should not be ARM specific, though it runs on an ARMv8 platform. But looking at the patch it has some errata workarounds triggered based on the CPU id (MIDR). That looks dodgy as it doesn't even check the full ID, only the variant part.
Okay, it's not ARM but an ARM64 driver, so that's your territory, not mine. :)
Well, feel free to enter this territory ;).

Quoted 7 lines

Anyway, what I meant is that such driver should not rely on CPU identification at all, especially since it does not require some specific CPU features but it tries to guess which SoC revision it is and which device (not CPU) bugs have been fixed. Better if they follow Mark R's idea to pass such information in DT for the device.
So maybe the revert is not too bad an idea.

The revert is completely sane.

FWIW, NAK to the original patch using the MIDR, or any other patch which uses the MIDR for device (not CPU) errata workarounds.

Mark.