ata: Add APM X-Gene SoC SATA host controller support

drivers/ata/sata_xgene.c new file +10 -0

+static int xgene_ahci_init_memram(struct xgene_ahci_context *ctx)
+{
+	void *diagcsr = ctx->csr_base + SATA_DIAG_OFFSET;
+	int timeout;
+	u32 val;
+
+	xgene_rd(diagcsr + REGSPEC_CFG_MEM_RAM_SHUTDOWN_ADDR, &val);
+	if (val == 0) {
+		dev_dbg(ctx->dev, "memory already released from shutdown\n");
+		return 0;

drivers/ata/sata_xgene.c new file +8 -0

+static unsigned int xgene_ahci_fill_sg(struct ata_queued_cmd *qc,
+				       void *cmd_tbl)
+{
+	struct scatterlist *sg;
+	struct ahci_sg *ahci_sg = cmd_tbl + AHCI_CMD_TBL_HDR_SZ;
+	unsigned int si;
+
+	/*

drivers/ata/sata_xgene.c new file +5 -0

+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "no AHCI MMIO resource address\n");
+		goto error;
+	}

drivers/ata/sata_xgene.c new file +17 -0

+#ifdef CONFIG_PM
+static int xgene_ahci_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct ata_host *host = dev_get_drvdata(&pdev->dev);
+	struct xgene_ahci_context *hpriv = host->private_data;
+	void __iomem *mmio = hpriv->mmio_base;
+	u32 ctl;
+	int rc;
+
+	dev_dbg(&pdev->dev, "suspend\n");
+
+	/*
+	 * AHCI spec rev1.1 section 8.3.3:
+	 * Software must disable interrupts prior to requesting a
+	 * transition of the HBA to D3 state.
+	 */
+	ctl = readl(mmio + HOST_CTL);

+static int xgene_ahci_resume(struct platform_device *pdev)

Quoted 10 lines

drivers/ata/sata_xgene.c

Quoted 10 lines

drivers/ata/sata_xgene.c new file +9 -0

+struct xgene_ahci_context {
+     struct ahci_host_priv  hpriv;
+     struct device *dev;
+     int irq;                /* IRQ */
+     void __iomem *csr_base; /* CSR base address of IP */
+     u64 csr_phys;           /* Physical address of CSR base address */
+     void __iomem *mmio_base; /* AHCI I/O base address */
+     u64 mmio_phys;          /* Physical address of MMIO base address */
+     void __iomem *ahbc_csr_base; /* Used for IOB flushing if non-zero

Quoted 9 lines

*/

Quoted 1 line

+ void __iomem *ahbc_io_base; /* Used for IOB flushing if non-zero *

I still think the flushing should be split out into a separate subsystem of some
sort, or into common code. Can you explain why it is needed in this driver, and
what other parts of your SoC also need to do the same thing?

[Loc Ho]
Due to HW bug, the read to flush does not function properly with the SATA and SD IP's. For this reason, to ensure that data has reached the DDR after reading the CI register of the AHCI, we need to flush the bus. This will ensure data for the corresponding CI bit are available before the SATA upper layer consumed the data. The chance of this happen is extremely lower but this will ensure 100% data consistency. As the SD IP is single operation at a time, it will never need this. Also, if it does, by enable the feature itself, any read on the SD register is enough to handle the bus flush. So, SD don't needs this as SATA will enable it. This will not be fix in the first generation of the chip and this explicit flush is required for SATA.

Quoted 1 line

Quoted 4 lines

Maybe we can have custom dma_map_ops for your platform so the flushing automatically
happens in dma_unmap_sg().

[Loc Ho]
We don't want this for other IP as it will affact performance for the other IP's.

Quoted 5 lines

Quoted 5 lines

drivers/ata/sata_xgene.c new file +5 -0

+static void xgene_wr_flush(void *addr, u32 val)
+{
+     writel(val, addr);
+     pr_debug("X-Gene SATA CSR WR: 0x%p value: 0x%08x\n", addr, val);
+     val = readl(addr);

Quoted 9 lines

Quoted 1 line

+}

This seems to be a rather expensive operation. Can you explain what the extra
flush is trying to achieve here? Normally you'd want to eliminate any readl()
from high-performance device drivers if you can. Is this used in any fast path?

[Loc Ho]
This is only for configuration barrier. Will not be used for IO.

Quoted 22 lines

drivers/ata/sata_xgene.c

Quoted 9 lines

drivers/ata/sata_xgene.c new file +9 -0

+static int xgene_ahci_init_memram(struct xgene_ahci_context *ctx)
+{
+     void *diagcsr = ctx->csr_base + SATA_DIAG_OFFSET;
+     int timeout;
+     u32 val;
+
+     xgene_rd(diagcsr + REGSPEC_CFG_MEM_RAM_SHUTDOWN_ADDR, &val);
+     if (val == 0) {
+             dev_dbg(ctx->dev, "memory already released from

shutdown\n");

Quoted 11 lines

drivers/ata/sata_xgene.c new file +11 -0

+             return 0;
+     }
+     dev_dbg(ctx->dev, "Release memory from shutdown\n");
+     /* SATA controller memory in shutdown. Remove from shutdown. */
+     xgene_wr_flush(diagcsr + REGSPEC_CFG_MEM_RAM_SHUTDOWN_ADDR, 0x00);
+     timeout = SATA_RESET_MEM_RAM_TO;
+     do {
+             xgene_rd(diagcsr + REGSPEC_BLOCK_MEM_RDY_ADDR, &val);
+             if (val != 0xFFFFFFFF)
+                     udelay(1);
+     } while (val != 0xFFFFFFFF && timeout-- > 0);

Quoted 5 lines

With SATA_RESET_MEM_RAM_TO == 100000, this is an awfully long busy-loop. Can you replace the udelay with usleep_range() providing a reasonable wide range, and the replace the retry count with a time_before(jiffies(), timeout) comparision?

[Loc Ho]
I reduce to 1000. It only takes about one or two reads.

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+/* Flush the IOB to ensure all SATA controller writes completed before
+   servicing the completed command. This is needed due to the

possibility

Quoted 1 line

+ that interrupt serviced before the data actually written to the

cache/DDR.

Review

Quoted 2 lines

(quoted diff) +2 -0

+   Writes from the IP to the CPU domain is not synchronized with the IRQ
+   line or the IP core toggled the CI bits before the data write

completed. */

Quoted 12 lines

drivers/ata/sata_xgene.c line 430

Quoted 6 lines

drivers/ata/sata_xgene.c new file +6 -0

+static int xgene_ahci_iob_flush(struct xgene_ahci_context *ctx)
+{
+     if (ctx->ahbc_io_base)
+             readl(ctx->ahbc_io_base);
+     return 0;
+}

Normally the synchronization should be guaranteed by using MSI interrupts rather than edge or level interrupt pins. Do you have MSI support in your hardware? Using that would get rid of this ugly hack and likely improve performance significantly.

[Loc Ho]
The SATA IP is built into the chip and NOT on the PCIe bus. The need for this is already mentioned above.

Quoted 22 lines

Quoted 22 lines

drivers/ata/sata_xgene.c

Quoted 9 lines

drivers/ata/sata_xgene.c new file +9 -0

+static int xgene_ahci_init_memram(struct xgene_ahci_context *ctx)
+{
+     void *diagcsr = ctx->csr_base + SATA_DIAG_OFFSET;
+     int timeout;
+     u32 val;
+
+     xgene_rd(diagcsr + REGSPEC_CFG_MEM_RAM_SHUTDOWN_ADDR, &val);
+     if (val == 0) {
+             dev_dbg(ctx->dev, "memory already released from

shutdown\n");

Quoted 11 lines

drivers/ata/sata_xgene.c new file +11 -0

+             return 0;
+     }
+     dev_dbg(ctx->dev, "Release memory from shutdown\n");
+     /* SATA controller memory in shutdown. Remove from shutdown. */
+     xgene_wr_flush(diagcsr + REGSPEC_CFG_MEM_RAM_SHUTDOWN_ADDR, 0x00);
+     timeout = SATA_RESET_MEM_RAM_TO;
+     do {
+             xgene_rd(diagcsr + REGSPEC_BLOCK_MEM_RDY_ADDR, &val);
+             if (val != 0xFFFFFFFF)
+                     udelay(1);
+     } while (val != 0xFFFFFFFF && timeout-- > 0);

Quoted 7 lines

Quoted 5 lines

With SATA_RESET_MEM_RAM_TO == 100000, this is an awfully long busy-loop. Can you replace the udelay with usleep_range() providing a reasonable wide range, and the replace the retry count with a time_before(jiffies(), timeout) comparision?

[Loc Ho]
I reduce to 1000. It only takes about one or two reads.

It's still over two miliseconds for the (unlikely) timeout in that case, plus the time it takes to do the 1000 reads. The important part of my comment was about using usleep_range(), which lets another process run in the meantime rather than blocking the CPU.

If you want to keep using a retry count rather than a timeout, don't call it "timeout".

Quoted 25 lines

Quoted 24 lines

Quoted 9 lines

Review

Quoted 3 lines

(quoted diff) +2 -0

+/* Flush the IOB to ensure all SATA controller writes completed before
+   servicing the completed command. This is needed due to the

possibility

Quoted 1 line

+ that interrupt serviced before the data actually written to the

cache/DDR.

Review

Quoted 2 lines

(quoted diff) +2 -0

+   Writes from the IP to the CPU domain is not synchronized with the IRQ
+   line or the IP core toggled the CI bits before the data write

completed. */

Quoted 12 lines

drivers/ata/sata_xgene.c line 430

Quoted 6 lines

drivers/ata/sata_xgene.c new file +6 -0

+static int xgene_ahci_iob_flush(struct xgene_ahci_context *ctx)
+{
+     if (ctx->ahbc_io_base)
+             readl(ctx->ahbc_io_base);
+     return 0;
+}

Normally the synchronization should be guaranteed by using MSI interrupts rather than edge or level interrupt pins. Do you have MSI support in your hardware? Using that would get rid of this ugly hack and likely improve performance significantly.

[Loc Ho]
The SATA IP is built into the chip and NOT on the PCIe bus. The need for this is already mentioned above.

Quoted 6 lines

I don't understand the comment about PCIe. MSI should work regardless of where the device resides in the system, unless you have the MSI controller integrated into the PCIe host, which would be a bug because it causes the same problem that you have on this device for any PCIe add-on device (the MSI would no longer flush the a PCIe bus master DMA transfer all the way to memory if the message is consumed by the PCIe host).

Documentation/devicetree/bindings/ata/apm-xgene.txt new file +6 -0

+SATA host controller nodes are defined to describe on-chip Serial ATA
+controllers. Each SATA controller (pair of ports) have its own node.
+
+Required properties:
+- compatible		: Shall be "apm,xgene-ahci"
+- reg			: First memory resource shall be the AHCI memory

Quoted 6 lines

Documentation/devicetree/bindings/ata/apm-xgene.txt new file +6 -0

+SATA host controller nodes are defined to describe on-chip Serial ATA
+controllers. Each SATA controller (pair of ports) have its own node.
+
+Required properties:
+- compatible         : Shall be "apm,xgene-ahci"
+- reg                        : First memory resource shall be the AHCI memory

Documentation/devicetree/bindings/ata/apm-xgene.txt line 18

Quoted 10 lines

Documentation/devicetree/bindings/ata/apm-xgene.txt new file +10 -0

+                       resource.
+                       Second memory resource shall be the host controller
+                       memory resource.
+- interrupt-parent   : Interrupt controller
+- interrupts         : Interrupt mapping for SATA host controller IRQ
+- clocks             : Reference to the clock entry
+- phys                       : PHY reference
+- phy-names          : Name of the PHY reference. The name should be
+                       formed by "sataphy" plus lower case hex for the
+                       lower 32-bit PHY CSR address (2nd memory resource)

I've tried to explain this before, but apparently you missed the point:

Any "*-names" property is supposed to be the identifier inside the device you are describing, *not* an identifier for what it points to, as that already has a name. The idea is that the driver can ask for phy (or clock, irq, memory, etc) "foo" here, with foo identifying the functionality in the device. If you only have one phy, you probably don't want to name it at all. If you imagine a device that was connected to two different phys depending on the mode it's used in (e.g. SATA-II vs SATA-6G), you would list this as

- phy-names : one name per references in the "phys" property.
valid names are "sata-ii" and "sata-6g".

If the generic phy subsystem requires you to name this, I'd suggest either changing the phy subsystem to make the name optional, or to just call it "serdes", as that is the kind of phy you are connected to.

Does this make more sense to you now?