[PATCH v2 2/4] ARM: sunxi: Add ahci-sunxi driver for the Allwinner SUNXi SoCs sata

On Sunday 05 January 2014, Hans de Goede wrote:

quoted

Also, please send the binding as a separate patch with Cc to
the devicetree-discuss mailing list.

Hmm, this contradicts what others are saying who have requested for
the binding docs to be part of the same commit as the driver (with
various other drivers). Note that I've cc-ed devicetree already.

Probably my fault then, I've been away for some time and may misremember
what we discussed. Maybe someone else can clarify.

quoted

quoted

+static int sunxi_ahci_phy_init(struct device *dev, void __iomem *reg_base)
+{
+	u32 reg_val;
+	int timeout;
+
+	/* This magic is from the original code */
+	writel(0, reg_base + AHCI_RWCR);
+	mdelay(5);

This function should probably be in a separate phy driver.  I would
very much hope that we can minimize the required code in an AHCI
driver and move code from this new file into the ahci-platform
driver. The clock, regulator and phy setup can all be optional
properties of the generic driver, and then there shouldn't
be much left that is sunxi specific.

Writing a phy driver, and extending ahci-platform to use that
was my original plan. But the phy really is part of the
ahci ip-block here, and not a separate ip-block. Its registers
are smack in the middle of the io-range for the ahci function.

I see. I wonder if the register layout is common with some other
implementation then. If it's part of the AHCI block, it's probably
not an Allwinner invention but comes from whoever implemented the
AHCI.

Also note that sunxi_ahci_pre_start_engine is rather sunxi
specific. Needing to put that in a generic ahci_platform driver
and only activating it for sunxi socs would only serve to
prove my point that at some point it is simply easier and
better to write a non generic platform glue driver when dealing
with exotic ahci ip blocks.

If we end up putting all sort of if soca do foo else if socb
do bar, else do nothing magic in ahci_platform.c I think we're
over generalizing. If something nicely fits as a generic
platform dev, by all means we should use a generic platform
driver, but that just won't work cleanly here.

Yes, but there may be some middle ground. I still think it would
be worthwhile to make the clock handling part of the common
ahci (or ahci-platform) driver and reuse that, since it seems
to be needed on a number of implementations. IIRC there is already
some inheritence model in libata that can be used to define
variations of drivers and have common parts done only once.

Actually I've just completed writing a phy driver for
the usbphy in the same soc, and a patch to extend ehci-platform
to take an optional clk and phy. So I completely agree with the
basic idea, it just does not seem doable cleanly in this case,
hence my choice to write a separate platform driver. For the
usb stuff (wip) see:
https://github.com/jwrdegoede/linux-sunxi/commit/4655dd01936f42d8a75da08a00af439e0a34eaf7
https://github.com/jwrdegoede/linux-sunxi/commit/bcb674859d015ff9e082829dbd5cf239b8b53d4a

Ah, very nice!

quoted

quoted

+static int sunxi_ahci_susp(struct device *dev)
+{
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+	struct sunxi_ahci *ahci = hpriv->plat_data;
+	int ret;
+
+	/*
+	 * AHCI spec rev1.1 section 8.3.3:
+	 * Software must disable interrupts prior to requesting a
+	 * transition of the HBA to D3 state.
+	 */
+	sunxi_clrbits(hpriv->mmio + HOST_CTL, HOST_IRQ_EN);
+
+	ret = ata_host_suspend(host, PMSG_SUSPEND);
+	if (ret)
+		return ret;
+
+	sunxi_ahci_disable_clks(ahci);
+
+	return 0;
+}

The only thing in here that seems sunxi-specific is the irq disabling
part. Can't you do this instead by calling disable_irq() and make
the function completely generic?

The irq disabling part actually is not sunxi specific, ahci_platform.c
has it too.

Ok. Can this function be shared in some way then, e.g. by exporting the
ahci-platform implementation?

	Arnd