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

arch/arm64/boot/dts/apm-storm.dtsi line 254

Quoted 13 lines

arch/arm64/boot/dts/apm-storm.dtsi +12 -0

+                       sata23clk: sata23clk at 1f22c000 {
+                               compatible = "apm,xgene-device-clock";
+                               #clock-cells = <1>;
+                               clocks = <&socplldiv2 0>;
+                               clock-names = "sata23clk";
 
+                       };
+
+                       sata45clk: sata45clk at 1f23c000 {
+                               compatible = "apm,xgene-device-clock";
+                               #clock-cells = <1>;
+                               clocks = <&socplldiv2 0>;
+                               clock-names = "sata45clk";

Something is wrong here: You have two devices with the same "compatible" string but using different "clock-names" strings. The binding document lists this as an optional property with the description "shall be the name of the device clock. If missing, use the device name", which doesn't seem to make any sense.

Please fix the binding and the existing users of this, and don't introduce any more broken instances. Since each device clock is documented to have only one parent anyway, please just make it an anonymous clock.

Okay I miss understood this... I will need to fix the X-Gene device parent clocks as well in an separate patch to the clock driver owner.

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

+static void xgene_ahci_enable_phy(struct xgene_ahci_context *ctx,
+				  int channel, int enable)
+{
+	void *mmio = ctx->mmio_base;
+	u32 val;
+
+	xgene_rd(mmio + PORTCFG_ADDR, &val);
+	val = PORTADDR_SET(val, channel == 0 ? 2 : 3);
+	xgene_wr_flush(mmio + PORTCFG_ADDR, val);
+	xgene_rd(mmio + PORTPHY1CFG_ADDR, &val);
+	val = PORTPHY1CFG_FRCPHYRDY_SET(val, enable);
+	xgene_wr(mmio + PORTPHY1CFG_ADDR, val);
+}
+
+static void xgene_ahci_set_phy_cfg(struct xgene_ahci_context *ctx, int channel)
+{
+	void *mmio = ctx->mmio_base;
+	u32 val;
+
+	dev_dbg(ctx->dev, "port configure mmio 0x%p channel %d\n",
+		mmio, channel);
+	xgene_rd(mmio + PORTCFG_ADDR, &val);
+	val = PORTADDR_SET(val, channel == 0 ? 2 : 3);
+	xgene_wr_flush(mmio + PORTCFG_ADDR, val);
+	/* Disable fix rate */
+	xgene_wr_flush(mmio + PORTPHY1CFG_ADDR, 0x0001fffe);
+	xgene_wr_flush(mmio + PORTPHY2CFG_ADDR, 0x5018461c);
+	xgene_wr_flush(mmio + PORTPHY3CFG_ADDR, 0x1c081907);

drivers/ata/sata_xgene.c line 36

Quoted 5 lines

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

+/* Controller who PHY shared with SGMII Ethernet PHY */
+#define XGENE_AHCI_SGMII_DTS         "apm,xgene-ahci-sgmii"
+
+/* Controller who PHY (internal reference clock macro) shared with PCIe */
+#define XGENE_AHCI_PCIE_DTS          "apm,xgene-ahci-pcie"

Kill these macros. I've commented on this in the past.

Also, there really shouldn't be any difference to the SATA driver based on what PHY is used, so the strings shouldn't be different either.

drivers/ata/sata_xgene.c line 58

Quoted 7 lines

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

+/* SATA host controller CSR */
+#define SLVRDERRATTRIBUTES_ADDR              0x00000000
+#define SLVWRERRATTRIBUTES_ADDR              0x00000004
+#define MSTRDERRATTRIBUTES_ADDR              0x00000008
+#define MSTWRERRATTRIBUTES_ADDR              0x0000000c
+#define BUSCTLREG_ADDR                       0x00000014

Are these strings taken literally from the data sheet? You can probably drop the _ADDR part.

drivers/ata/sata_xgene.c line 62

Quoted 5 lines

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

+#define  MSTAWAUX_COHERENT_BYPASS_SET(dst, src) \
+             (((dst) & ~0x00000002) | (((u32)(src)<<1) & 0x00000002))
+#define  MSTARAUX_COHERENT_BYPASS_SET(dst, src) \
+             (((dst) & ~0x00000001) | (((u32)(src)) & 0x00000001))

The macros are only used once and don't really help readability. Just open-code them. If you have complex macros like these and use them multiple times, it may be better to use an inline function.

Quoted 33 lines

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

+static void xgene_ahci_enable_phy(struct xgene_ahci_context *ctx,
+                               int channel, int enable)
+{
+     void *mmio = ctx->mmio_base;
+     u32 val;
+
+     xgene_rd(mmio + PORTCFG_ADDR, &val);
+     val = PORTADDR_SET(val, channel == 0 ? 2 : 3);
+     xgene_wr_flush(mmio + PORTCFG_ADDR, val);
+     xgene_rd(mmio + PORTPHY1CFG_ADDR, &val);
+     val = PORTPHY1CFG_FRCPHYRDY_SET(val, enable);
+     xgene_wr(mmio + PORTPHY1CFG_ADDR, val);
+}
+
+static void xgene_ahci_set_phy_cfg(struct xgene_ahci_context *ctx, int channel)
+{
+     void *mmio = ctx->mmio_base;
+     u32 val;
+
+     dev_dbg(ctx->dev, "port configure mmio 0x%p channel %d\n",
+             mmio, channel);
+     xgene_rd(mmio + PORTCFG_ADDR, &val);
+     val = PORTADDR_SET(val, channel == 0 ? 2 : 3);
+     xgene_wr_flush(mmio + PORTCFG_ADDR, val);
+     /* Disable fix rate */
+     xgene_wr_flush(mmio + PORTPHY1CFG_ADDR, 0x0001fffe);
+     xgene_wr_flush(mmio + PORTPHY2CFG_ADDR, 0x5018461c);
+     xgene_wr_flush(mmio + PORTPHY3CFG_ADDR, 0x1c081907);
+     xgene_wr_flush(mmio + PORTPHY4CFG_ADDR, 0x1c080815);
+     xgene_rd(mmio + PORTPHY5CFG_ADDR, &val);
+     /* Window negotiation 0x800 to 0x400 */
+     val = PORTPHY5CFG_RTCHG_SET(val, 0x300);

drivers/ata/sata_xgene.c line 295

Quoted 6 lines

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

+     xgene_wr_flush(mmio + PORTPHY5CFG_ADDR, val);
+     xgene_rd(mmio + PORTAXICFG_ADDR, &val);
+     val = PORTAXICFG_EN_CONTEXT_SET(val, 0x1); /* enable context mgmt */
+     val = PORTAXICFG_OUTTRANS_SET(val, 0xe); /* Outstanding */
+     xgene_wr_flush(mmio + PORTAXICFG_ADDR, val);
+}

This looks like it should be part of the PHY driver?

drivers/ata/sata_xgene.c line 844

Quoted 8 lines

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

+     /*
+      * When both ACPI and DTS are enabled, custom ACPI built-in ACPI
+      * table, and booting via DTS, we need to skip the probe of the
+      * built-in ACPI table probe.
+      */
+     if (!efi_enabled(EFI_BOOT) && dev->of_node == NULL)
+             return -ENODEV;

I think I've commented on this one before too. The comment talks about ACPI, but the code is about EFI, which is completely unrelated. Neither the code nor the comment seems to make sense here and you wouldn't be in this function in the first place if the device is not defined in ACPI or in DT.

drivers/ata/sata_xgene.c line 879

Quoted 7 lines

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

+     /* Can't use devm_ioremap_resource due to overlapping region */
+     hpriv->csr_base = devm_ioremap(dev, res->start, resource_size(res));
+     if (!hpriv->csr_base) {
+             dev_err(dev, "can't map %pR\n", res);
+             return -ENOMEM;
+     }

Why are the regions overlapping? That should not happen!

drivers/ata/sata_xgene.c line 926

Quoted 9 lines

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

+     /* Select ATA */
+     if (of_device_is_compatible(pdev->dev.of_node,
+             XGENE_AHCI_SGMII_DTS)) {
+             if (xgene_ahci_mux_select(hpriv)) {
+                     dev_err(dev, "SATA mux selection failed\n");
+                     return -ENODEV;
+             }
+     }

What kind of mux is this? Why does the SATA driver need to care about it? It sounds like this should be part of the pinctrl driver.

drivers/ata/sata_xgene.c line 1029

Quoted 12 lines

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

+     /* Setup DMA mask */
+     rc = dma_set_mask(dev, DMA_BIT_MASK(64));
+     if (rc) {
+             dev_err(dev, "Unable to set dma mask\n");
+             goto error;
+     }
+     rc = dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
+     if (rc) {
+             dev_err(dev, "Unable to set dma coherent mask\n");
+             goto error;
+     }

dma_set_mask_and_coherent?

drivers/ata/sata_xgene.c line 1047

Quoted 7 lines

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

+
+static const struct acpi_device_id xgene_ahci_acpi_match[] = {
+     {"APMC0D00", 0},
+     {},
+};
+MODULE_DEVICE_TABLE(acpi, xgene_ahci_acpi_match);

Just drop the ACPI part for now. It's clear that the driver won't work with ACPI in this state, since it would need to handle the PHY and clock management completely differently. It's better to add the code once it has a chance to actually work.