Hi,

quoted

+#define pdata_to_ctx(x) container_of(x, struct xgene_ahci_context, plat_data)
+
+struct xgene_ahci_context {
+     struct ahci_platform_data plat_data;

plat_data is used only to get to struct xgene_ahci_context instance
so it can be removed (especially since we want to remove struct
ahci_platform_data altogether in the long-term) and hpriv->plat_data
should be set to point to context instance itself.  Actually, this
seems to be the case already as hpriv->plat_data is set to hplat_data
(not to &hplat_data->plat_data) in xgene_ahci_probe() so I wonder how
does this driver work currently?

I will remove the plat_data. .

quoted

+/**
+ * xgene_ahci_read_id - Read ID data from the specified device
+ * @dev: device
+ * @tf: proposed taskfile
+ * @id: data buffer
+ *
+ * This custom read ID function is required due to the fact that the HW
+ * does not support DEVSLP and the controller state machine may get stuck
+ * after processing the ID query command.
+ */
+static unsigned int xgene_ahci_read_id(struct ata_device *dev,
+                                    struct ata_taskfile *tf, u16 *id)
+{
+     u32 err_mask;
+     void __iomem *port_mmio = ahci_port_base(dev->link->ap);
+
+     err_mask = ata_do_dev_read_id(dev, tf, id);
+     if (err_mask)
+             return err_mask;
+
+     /*
+      * Mask reserved area. Bit78 spec of Link Power Management

Word78?

Yes...

quoted

+      * bit15-8: reserved
+      * bit7: NCQ autosence
+      * bit6: Software settings preservation supported
+      * bit5: reserved
+      * bit4: In-order sata delivery supported
+      * bit3: DIPM requests supported
+      * bit2: DMA Setup FIS Auto-Activate optimization supported
+      * bit1: DMA Setup FIX non-Zero buffer offsets supported
+      * bit0: Reserved
+      *
+      * Clear reserved bit (DEVSLP bit) as we don't support DEVSLP
+      */
+     id[78] &= 0x00FF;

I will also fix this up by bit mask off.

quoted

+
+     /* HW requires toggle of the clock */
+     ahci_platform_disable_clks(hpriv);
+     rc = ahci_platform_enable_clks(hpriv);

Why is this needed (extra clocks disable->enable sequence)?

This is an HW errata with the clock. If I don't give the clock an full
cycle, it doesn't work.

-Loc