ata: ahci_platform related cleanups

[ v1 was named "ata: add remaining new-style AHCI platform drivers"
but we are down from 2 drivers to 1 driver so the title of the patch series no longer was valid :) ]

Hi,

This patch series:
- fixes ahci_platform_data->suspend() handling
- moves library AHCI platform code to its own file
- adds a new-style AHCI platform driver for DaVinci DA850 AHCI controller

Changes since v1:
- dropped no longer needed patch #4 (ST SPEAr1340 AHCI controller driver)
- adressed review comments for patch #3 from Sekhar Nori
- updated patch descriptions

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Bartlomiej Zolnierkiewicz (3):
ata: ahci_platform: fix ahci_platform_data->suspend method handling
ata: move library code from ahci_platform.c to libahci_platform.c
ata: add new-style AHCI platform driver for DaVinci DA850 AHCI
controller

 arch/arm/mach-davinci/devices-da8xx.c |  99 +------
 drivers/ata/Kconfig                   |  14 +-
 drivers/ata/Makefile                  |   9 +-
 drivers/ata/ahci_da850.c              | 116 ++++++++
 drivers/ata/ahci_platform.c           | 508 -------------------------------
 drivers/ata/libahci_platform.c        | 541 ++++++++++++++++++++++++++++++++++
 6 files changed, 681 insertions(+), 606 deletions(-)
 create mode 100644 drivers/ata/ahci_da850.c
 create mode 100644 drivers/ata/libahci_platform.c

1.8.2.3

Looking at ST SPEAr1340 AHCI code (the only user of the deprecated pdata->suspend and pdata->resume) it is obvious the we should return after calling pdata->suspend() only if the function have returned non-zero return value. The code has been broken since commit 1e70c2 ("ata/ahci_platform: Add clock framework support"). Fix it.

---
v2:
- updated patch description

 drivers/ata/ahci_platform.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

index 70fbf66..7bd6adf 100644
@@ -521,12 +521,19 @@ int ahci_platform_suspend(struct device *dev)
 	if (rc)
 		return rc;
 
-	if (pdata && pdata->suspend)
-		return pdata->suspend(dev);
+	if (pdata && pdata->suspend) {
+		rc = pdata->suspend(dev);
+		if (rc)
+			goto resume_host;
+	}
 
 	ahci_platform_disable_resources(hpriv);
 
 	return 0;
+
+resume_host:
+	ahci_platform_resume_host(dev);
+	return rc;
 }
 EXPORT_SYMBOL_GPL(ahci_platform_suspend);
 

1.8.2.3

Move AHCI platform library code from ahci_platform.c to libahci_platform.c and fix dependencies for ahci_st, ahci_imx and ahci_sunxi drivers.

---
v2:
- updated patch description

 drivers/ata/Kconfig            |   5 +-
 drivers/ata/Makefile           |   8 +-
 drivers/ata/ahci_platform.c    | 515 ---------------------------------------
 drivers/ata/libahci_platform.c | 541 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 547 insertions(+), 522 deletions(-)
 create mode 100644 drivers/ata/libahci_platform.c

index c4db48c..371e8890 100644
@@ -99,7 +99,6 @@ config SATA_AHCI_PLATFORM
 
 config AHCI_ST
 	tristate "ST AHCI SATA support"
-	depends on SATA_AHCI_PLATFORM
 	depends on ARCH_STI
 	help
 	  This option enables support for ST AHCI SATA controller.

@@ -108,7 +107,7 @@ config AHCI_ST
 
 config AHCI_IMX
 	tristate "Freescale i.MX AHCI SATA support"
-	depends on SATA_AHCI_PLATFORM && MFD_SYSCON
+	depends on MFD_SYSCON
 	help
 	  This option enables support for the Freescale i.MX SoC's
 	  onboard AHCI SATA.

@@ -117,7 +116,7 @@ config AHCI_IMX
 
 config AHCI_SUNXI
 	tristate "Allwinner sunxi AHCI SATA support"
-	depends on ARCH_SUNXI && SATA_AHCI_PLATFORM
+	depends on ARCH_SUNXI
 	help
 	  This option enables support for the Allwinner sunxi SoC's
 	  onboard AHCI SATA.

index e20148c..6123e64 100644
@@ -4,15 +4,15 @@ obj-$(CONFIG_ATA)		+= libata.o
 # non-SFF interface
 obj-$(CONFIG_SATA_AHCI)		+= ahci.o libahci.o
 obj-$(CONFIG_SATA_ACARD_AHCI)	+= acard-ahci.o libahci.o
-obj-$(CONFIG_SATA_AHCI_PLATFORM) += ahci_platform.o libahci.o
+obj-$(CONFIG_SATA_AHCI_PLATFORM) += ahci_platform.o libahci.o libahci_platform.o
 obj-$(CONFIG_SATA_FSL)		+= sata_fsl.o
 obj-$(CONFIG_SATA_INIC162X)	+= sata_inic162x.o
 obj-$(CONFIG_SATA_SIL24)	+= sata_sil24.o
 obj-$(CONFIG_SATA_DWC)		+= sata_dwc_460ex.o
 obj-$(CONFIG_SATA_HIGHBANK)	+= sata_highbank.o libahci.o
-obj-$(CONFIG_AHCI_IMX)		+= ahci_imx.o
-obj-$(CONFIG_AHCI_SUNXI)	+= ahci_sunxi.o
-obj-$(CONFIG_AHCI_ST)		+= ahci_st.o
+obj-$(CONFIG_AHCI_IMX)		+= ahci_imx.o libahci.o libahci_platform.o
+obj-$(CONFIG_AHCI_SUNXI)	+= ahci_sunxi.o libahci.o libahci_platform.o
+obj-$(CONFIG_AHCI_ST)		+= ahci_st.o libahci.o libahci_platform.o
 
 # SFF w/ custom DMA
 obj-$(CONFIG_PDC_ADMA)		+= pdc_adma.o

1.8.2.3

Add the new ahci_da850 host driver and remove the deprecated ahci_platform_data platform code.

Please note that the new driver doesn't have the superfluous clock control code as clock is already handled by the generic AHCI platform library code.

---
v2:
- dropped the experimental tag from the config option help
- fixed SYSCFG1 memory resource handling
- hardcoded the multiplier data and added a note about it
- used readl()/writel() instead of __raw_readl()/__raw_writel()
- dropped the superflous clock control
- cleaned up da850_sata_init()
- changed the platform device name and removed the platform ids table
- removed the deprecated ahci_platform_data platform code
- updated the patch description

 arch/arm/mach-davinci/devices-da8xx.c |  99 +++--------------------------
 drivers/ata/Kconfig                   |   9 +++
 drivers/ata/Makefile                  |   1 +
 drivers/ata/ahci_da850.c              | 116 ++++++++++++++++++++++++++++++++++
 4 files changed, 134 insertions(+), 91 deletions(-)
 create mode 100644 drivers/ata/ahci_da850.c

index 0486cdf2..72bb8d6 100644
@@ -1020,7 +1020,6 @@ int __init da8xx_register_spi_bus(int instance, unsigned num_chipselect)
 }
 
 #ifdef CONFIG_ARCH_DAVINCI_DA850
-
 static struct resource da850_sata_resources[] = {
 	{
 		.start	= DA850_SATA_BASE,

@@ -1028,103 +1027,22 @@ static struct resource da850_sata_resources[] = {
 		.flags	= IORESOURCE_MEM,
 	},
 	{
+		.start	= DA8XX_SYSCFG1_BASE,
+		.end	= DA8XX_SYSCFG1_BASE + SZ_4K - 1,
+		.flags	= IORESOURCE_MEM,
+	},
+	{
 		.start	= IRQ_DA850_SATAINT,
 		.flags	= IORESOURCE_IRQ,
 	},
 };
 
-/* SATA PHY Control Register offset from AHCI base */
-#define SATA_P0PHYCR_REG	0x178
-
-#define SATA_PHY_MPY(x)		((x) << 0)
-#define SATA_PHY_LOS(x)		((x) << 6)
-#define SATA_PHY_RXCDR(x)	((x) << 10)
-#define SATA_PHY_RXEQ(x)	((x) << 13)
-#define SATA_PHY_TXSWING(x)	((x) << 19)
-#define SATA_PHY_ENPLL(x)	((x) << 31)
-
-static struct clk *da850_sata_clk;
-static unsigned long da850_sata_refclkpn;
-
-/* Supported DA850 SATA crystal frequencies */
-#define KHZ_TO_HZ(freq) ((freq) * 1000)
-static unsigned long da850_sata_xtal[] = {
-	KHZ_TO_HZ(300000),
-	KHZ_TO_HZ(250000),
-	0,			/* Reserved */
-	KHZ_TO_HZ(187500),
-	KHZ_TO_HZ(150000),
-	KHZ_TO_HZ(125000),
-	KHZ_TO_HZ(120000),
-	KHZ_TO_HZ(100000),
-	KHZ_TO_HZ(75000),
-	KHZ_TO_HZ(60000),
-};
-
-static int da850_sata_init(struct device *dev, void __iomem *addr)
-{
-	int i, ret;
-	unsigned int val;
-
-	da850_sata_clk = clk_get(dev, NULL);
-	if (IS_ERR(da850_sata_clk))
-		return PTR_ERR(da850_sata_clk);
-
-	ret = clk_prepare_enable(da850_sata_clk);
-	if (ret)
-		goto err0;
-
-	/* Enable SATA clock receiver */
-	val = __raw_readl(DA8XX_SYSCFG1_VIRT(DA8XX_PWRDN_REG));
-	val &= ~BIT(0);
-	__raw_writel(val, DA8XX_SYSCFG1_VIRT(DA8XX_PWRDN_REG));
-
-	/* Get the multiplier needed for 1.5GHz PLL output */
-	for (i = 0; i < ARRAY_SIZE(da850_sata_xtal); i++)
-		if (da850_sata_xtal[i] == da850_sata_refclkpn)
-			break;
-
-	if (i == ARRAY_SIZE(da850_sata_xtal)) {
-		ret = -EINVAL;
-		goto err1;
-	}
-
-	val = SATA_PHY_MPY(i + 1) |
-		SATA_PHY_LOS(1) |
-		SATA_PHY_RXCDR(4) |
-		SATA_PHY_RXEQ(1) |
-		SATA_PHY_TXSWING(3) |
-		SATA_PHY_ENPLL(1);
-
-	__raw_writel(val, addr + SATA_P0PHYCR_REG);
-
-	return 0;
-
-err1:
-	clk_disable_unprepare(da850_sata_clk);
-err0:
-	clk_put(da850_sata_clk);
-	return ret;
-}
-
-static void da850_sata_exit(struct device *dev)
-{
-	clk_disable_unprepare(da850_sata_clk);
-	clk_put(da850_sata_clk);
-}
-
-static struct ahci_platform_data da850_sata_pdata = {
-	.init	= da850_sata_init,
-	.exit	= da850_sata_exit,
-};
-
 static u64 da850_sata_dmamask = DMA_BIT_MASK(32);
 
 static struct platform_device da850_sata_device = {
-	.name	= "ahci",
+	.name	= "ahci_da850",
 	.id	= -1,
 	.dev	= {
-		.platform_data		= &da850_sata_pdata,
 		.dma_mask		= &da850_sata_dmamask,
 		.coherent_dma_mask	= DMA_BIT_MASK(32),
 	},

@@ -1134,9 +1052,8 @@ static struct platform_device da850_sata_device = {
 
 int __init da850_register_sata(unsigned long refclkpn)
 {
-	da850_sata_refclkpn = refclkpn;
-	if (!da850_sata_refclkpn)
-		return -EINVAL;
+	/* please see comment in drivers/ata/ahci_da850.c */
+	BUG_ON(refclkpn != 100 * 1000 * 1000);
 
 	return platform_device_register(&da850_sata_device);
 }

index 371e8890..4b71e73 100644
@@ -97,6 +97,15 @@ config SATA_AHCI_PLATFORM
 
 	  If unsure, say N.
 
+config AHCI_DA850
+	tristate "DaVinci DA850 AHCI SATA support"
+	depends on ARCH_DAVINCI_DA850
+	help
+	  This option enables support for the DaVinci DA850 SoC's
+	  onboard AHCI SATA.
+
+	  If unsure, say N.
+
 config AHCI_ST
 	tristate "ST AHCI SATA support"
 	depends on ARCH_STI

index 6123e64..0646d83 100644
@@ -10,6 +10,7 @@ obj-$(CONFIG_SATA_INIC162X)	+= sata_inic162x.o
 obj-$(CONFIG_SATA_SIL24)	+= sata_sil24.o
 obj-$(CONFIG_SATA_DWC)		+= sata_dwc_460ex.o
 obj-$(CONFIG_SATA_HIGHBANK)	+= sata_highbank.o libahci.o
+obj-$(CONFIG_AHCI_DA850)	+= ahci_da850.o libahci.o libahci_platform.o
 obj-$(CONFIG_AHCI_IMX)		+= ahci_imx.o libahci.o libahci_platform.o
 obj-$(CONFIG_AHCI_SUNXI)	+= ahci_sunxi.o libahci.o libahci_platform.o
 obj-$(CONFIG_AHCI_ST)		+= ahci_st.o libahci.o libahci_platform.o

1.8.2.3

Looks much better now and re-tested it on DA850 EVM. Few issues pointed out below.

I prefer not to mix platform and driver together in one patch. If you separate out the platform changes, I can take then through my tree with out risk of conflicts. The platform changes can come after the driver is introduced so there is no breakage.

index 72bb8d6..56ea41d 100644
@@ -1027,8 +1027,8 @@ static struct resource da850_sata_resources[] = {
 		.flags	= IORESOURCE_MEM,
 	},
 	{
-		.start	= DA8XX_SYSCFG1_BASE,
-		.end	= DA8XX_SYSCFG1_BASE + SZ_4K - 1,
+		.start	= DA8XX_SYSCFG1_BASE + DA8XX_PWRDN_REG,
+		.end	= DA8XX_SYSCFG1_BASE + DA8XX_PWRDN_REG + 0x3,
 		.flags	= IORESOURCE_MEM,
 	},
 	{

index 899270a..2c83613 100644
@@ -16,8 +16,6 @@
 #include <linux/ahci_platform.h>
 #include "ahci.h"
 
-#define DA8XX_PWRDN_REG		0x18
-
 /* SATA PHY Control Register offset from AHCI base */
 #define SATA_P0PHYCR_REG	0x178
 

@@ -37,15 +35,15 @@
  */
 #define DA850_SATA_CLK_MULTIPLIER	7
 
-static void da850_sata_init(struct device *dev, void __iomem *syscfg1_base,
+static void da850_sata_init(struct device *dev, void __iomem *pwrdn_reg,
 			    void __iomem *ahci_base)
 {
 	unsigned int val;
 
 	/* Enable SATA clock receiver */
-	val = readl(syscfg1_base + DA8XX_PWRDN_REG);
+	val = readl(pwrdn_reg);
 	val &= ~BIT(0);
-	writel(val, syscfg1_base + DA8XX_PWRDN_REG);
+	writel(val, pwrdn_reg);
 
 	val = SATA_PHY_MPY(DA850_SATA_CLK_MULTIPLIER + 1) | SATA_PHY_LOS(1) |
 	      SATA_PHY_RXCDR(4) | SATA_PHY_RXEQ(1) | SATA_PHY_TXSWING(3) |

@@ -66,7 +64,7 @@ static int ahci_da850_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct ahci_host_priv *hpriv;
 	struct resource *res;
-	void __iomem *syscfg1_base;
+	void __iomem *pwrdn_reg;
 	int rc;
 
 	hpriv = ahci_platform_get_resources(pdev);

@@ -81,11 +79,11 @@ static int ahci_da850_probe(struct platform_device *pdev)
 	if (!res)
 		goto disable_resources;
 
-	syscfg1_base = devm_ioremap(dev, res->start, resource_size(res));
-	if (!syscfg1_base)
+	pwrdn_reg = devm_ioremap(dev, res->start, resource_size(res));
+	if (!pwrdn_reg)
 		goto disable_resources;
 
-	da850_sata_init(dev, syscfg1_base, hpriv->mmio);
+	da850_sata_init(dev, pwrdn_reg, hpriv->mmio);
 
 	rc = ahci_platform_init_host(pdev, hpriv, &ahci_da850_port_info, 0, 0);
 	if (rc)

index 2ab0043..85399c9 100644
@@ -472,7 +472,7 @@ static struct clk_lookup da850_clks[] = {
 	CLK("spi_davinci.0",	NULL,		&spi0_clk),
 	CLK("spi_davinci.1",	NULL,		&spi1_clk),
 	CLK("vpif",		NULL,		&vpif_clk),
-	CLK("ahci",		NULL,		&sata_clk),
+	CLK("ahci_da850",		NULL,		&sata_clk),
 	CLK("davinci-rproc.0",	NULL,		&dsp_clk),
 	CLK("ehrpwm",		"fck",		&ehrpwm_clk),
 	CLK("ehrpwm",		"tbclk",	&ehrpwm_tbclk),

Hi,

I assume that the spear patch has been dropped since the spear code has already been converted now and thus is no longer using ahci_platform_data ?

In that case I think it would best to replace patch 1 with a patch just dropping the suspend and resume callbacks from ahci_platform_data, as they then no longer have any users.

Regards,

Hans

Hi,

Yes, the SPEAr platform AHCI code has been replaced by a proper PHY driver.

I have this on my TODO but since the SPEAr changes are not yet merged upstream this have to wait a bit.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics