[PATCH v3 0/8] Serial ATA support for NVIDIA Tegra124

[PATCH v3 0/8] Serial ATA support for NVIDIA Tegra124

[Mikko Perttunen]

Hi,

This series adds support for the onboard AHCI-compliant Serial ATA
controller found on Tegra124 systems-on-chip.

A branch containing the series is located at 
	git://github.com/cyndis/linux.git
branch ahci-rel-v3.

Changes in v3:
- Fixed style issues in driver
- Changed resource management to use libahci_platform's functions
  I opted to not add reset_control support to libahci_platform yet
  in the interest of getting this series into 3.17. Adding support
  probably requires breaking the existing API and getting ACKs from
  the maintainers of each platform driver.
- Removed code that reads the calibration set to use from fuses.
  The new-style fuse driver this requires is also going in for 3.17,
  so this should make merging easier. For now, the driver defaults
  to assuming FUSE_SATA_CALIB = 0, which works at least for my testing
  hardware. This should be changed to read the actual fuse once the
  fuse series has landed.

Mikko Perttunen (8):
  of: Add NVIDIA Tegra SATA controller binding
  ARM: tegra: Add SATA controller to Tegra124 device tree
  ARM: tegra: Add SATA and SATA power to Jetson TK1 device tree
  clk: tegra: Enable hardware control of SATA PLL
  clk: tegra: Add SATA clocks to Tegra124 initialization table
  ata: ahci_platform: Increase AHCI_MAX_CLKS to 4
  ata: Add support for the Tegra124 SATA controller
  ARM: tegra: Add options for Tegra AHCI support to tegra_defconfig

 .../devicetree/bindings/ata/tegra-sata.txt         |  30 ++
 arch/arm/boot/dts/tegra124-jetson-tk1.dts          |  35 ++
 arch/arm/boot/dts/tegra124.dtsi                    |  25 ++
 arch/arm/configs/tegra_defconfig                   |   3 +
 drivers/ata/Kconfig                                |   9 +
 drivers/ata/Makefile                               |   1 +
 drivers/ata/ahci.h                                 |   2 +-
 drivers/ata/ahci_tegra.c                           | 406 +++++++++++++++++++++
 drivers/clk/tegra/clk-pll.c                        |   8 +
 drivers/clk/tegra/clk-tegra124.c                   |   2 +
 10 files changed, 520 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/ata/tegra-sata.txt
 create mode 100644 drivers/ata/ahci_tegra.c

-- 
1.8.1.5

[PATCH v3 1/8] of: Add NVIDIA Tegra SATA controller binding

[Mikko Perttunen]

This patch adds device tree binding documentation for the SATA
controller found on NVIDIA Tegra SoCs.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 .../devicetree/bindings/ata/tegra-sata.txt         | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ata/tegra-sata.txt

diff --git a/Documentation/devicetree/bindings/ata/tegra-sata.txt b/Documentation/devicetree/bindings/ata/tegra-sata.txt
new file mode 100644
index 0000000..946f207
--- /dev/null
+++ b/Documentation/devicetree/bindings/ata/tegra-sata.txt
@@ -0,0 +1,30 @@
+Tegra124 SoC SATA AHCI controller
+
+Required properties :
+- compatible : "nvidia,tegra124-ahci".
+- reg : Should contain 2 entries:
+  - AHCI register set (SATA BAR5)
+  - SATA register set
+- interrupts : Defines the interrupt used by SATA
+- clocks : Must contain an entry for each entry in clock-names.
+  See ../clocks/clock-bindings.txt for details.
+- clock-names : Must include the following entries:
+  - sata
+  - sata-oob
+  - cml1
+  - pll_e
+- resets : Must contain an entry for each entry in reset-names.
+  See ../reset/reset.txt for details.
+- reset-names : Must include the following entries:
+  - sata
+  - sata-oob
+  - sata-cold
+- phys : Must contain an entry for each entry in phy-names.
+  See ../phy/phy-bindings.txt for details.
+- phy-names : Must include the following entries:
+  - sata-phy : XUSB PADCTL SATA PHY
+- hvdd-supply : Defines the SATA HVDD regulator
+- vddio-supply : Defines the SATA VDDIO regulator
+- avdd-supply : Defines the SATA AVDD regulator
+- target-5v-supply : Defines the SATA 5V power regulator
+- target-12v-supply : Defines the SATA 12V power regulator
-- 
1.8.1.5

[PATCH v3 2/8] ARM: tegra: Add SATA controller to Tegra124 device tree

[Mikko Perttunen]

This adds the integrated AHCI-compliant Serial ATA controller present
in Tegra124 systems-on-chip to the Tegra124 device tree.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 arch/arm/boot/dts/tegra124.dtsi | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
index 03916ef..7d3c48a 100644
--- a/arch/arm/boot/dts/tegra124.dtsi
+++ b/arch/arm/boot/dts/tegra124.dtsi
@@ -756,6 +756,31 @@
 		status = "disabled";
 	};
 
+	sata@0,70020000 {
+		compatible = "nvidia,tegra124-ahci";
+
+		reg = <0x0 0x70027000 0x0 0x2000>, /* AHCI */
+			<0x0 0x70020000 0x0 0x7000>; /* SATA */
+
+		interrupts = <GIC_SPI 23 IRQ_TYPE_LEVEL_HIGH>;
+
+		clocks = <&tegra_car TEGRA124_CLK_SATA>,
+			<&tegra_car TEGRA124_CLK_SATA_OOB>,
+			<&tegra_car TEGRA124_CLK_CML1>,
+			<&tegra_car TEGRA124_CLK_PLL_E>;
+		clock-names = "sata", "sata-oob", "cml1", "pll_e";
+
+		resets = <&tegra_car 124>,
+			<&tegra_car 123>,
+			<&tegra_car 129>;
+		reset-names = "sata", "sata-oob", "sata-cold";
+
+		phys = <&padctl TEGRA_XUSB_PADCTL_SATA>;
+		phy-names = "sata-phy";
+
+		status = "disabled";
+	};
+
 	cpus {
 		#address-cells = <1>;
 		#size-cells = <0>;
-- 
1.8.1.5

[PATCH v3 3/8] ARM: tegra: Add SATA and SATA power to Jetson TK1 device tree

[Mikko Perttunen]

This enables the integrated SATA controller on the Tegra124 system-on-chip
on the Jetson TK1 board and adds regulators for the onboard Molex connector
commonly used to power SATA devices. The regulators are marked always-on
since they can be used for other purposes than powering SATA devices.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 arch/arm/boot/dts/tegra124-jetson-tk1.dts | 35 +++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/arch/arm/boot/dts/tegra124-jetson-tk1.dts b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
index 624b0fb..a4221e2 100644
--- a/arch/arm/boot/dts/tegra124-jetson-tk1.dts
+++ b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
@@ -1687,6 +1687,18 @@
 		vbus-supply = <&vdd_usb3_vbus>;
 	};
 
+	/* Serial ATA */
+	sata@0,70020000 {
+		status = "okay";
+
+		hvdd-supply = <&vdd_3v3_lp0>;
+		vddio-supply = <&vdd_1v05_run>;
+		avdd-supply = <&vdd_1v05_run>;
+
+		target-5v-supply = <&vdd_5v0_sata>;
+		target-12v-supply = <&vdd_12v0_sata>;
+	};
+
 	clocks {
 		compatible = "simple-bus";
 		#address-cells = <1>;
@@ -1828,6 +1840,29 @@
 			enable-active-high;
 			vin-supply = <&vdd_5v0_sys>;
 		};
+
+		/* Molex power connector */
+		vdd_5v0_sata: regulator@13 {
+			compatible = "regulator-fixed";
+			reg = <13>;
+			regulator-name = "+5V_SATA";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+			gpio = <&gpio TEGRA_GPIO(EE, 2) GPIO_ACTIVE_HIGH>;
+			enable-active-high;
+			vin-supply = <&vdd_5v0_sys>;
+		};
+
+		vdd_12v0_sata: regulator@14 {
+			compatible = "regulator-fixed";
+			reg = <14>;
+			regulator-name = "+12V_SATA";
+			regulator-min-microvolt = <12000000>;
+			regulator-max-microvolt = <12000000>;
+			gpio = <&gpio TEGRA_GPIO(EE, 2) GPIO_ACTIVE_HIGH>;
+			enable-active-high;
+			vin-supply = <&vdd_mux>;
+		};
 	};
 
 	sound {
-- 
1.8.1.5

[PATCH v3 4/8] clk: tegra: Enable hardware control of SATA PLL

[Mikko Perttunen]

This makes the SATA PLL be controlled by hardware instead of software.
This is required for working SATA support.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
Already ACKed.

 drivers/clk/tegra/clk-pll.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
index 637b62c..f070c36 100644
--- a/drivers/clk/tegra/clk-pll.c
+++ b/drivers/clk/tegra/clk-pll.c
@@ -110,6 +110,9 @@
 #define XUSBIO_PLL_CFG0_SEQ_ENABLE		BIT(24)
 #define XUSBIO_PLL_CFG0_SEQ_START_STATE		BIT(25)
 
+#define SATA_PLL_CFG0		0x490
+#define SATA_PLL_CFG0_PADPLL_RESET_SWCTL	BIT(0)
+
 #define PLLE_MISC_PLLE_PTS	BIT(8)
 #define PLLE_MISC_IDDQ_SW_VALUE	BIT(13)
 #define PLLE_MISC_IDDQ_SW_CTRL	BIT(14)
@@ -1361,6 +1364,11 @@ static int clk_plle_tegra114_enable(struct clk_hw *hw)
 	val |= XUSBIO_PLL_CFG0_SEQ_ENABLE;
 	pll_writel(val, XUSBIO_PLL_CFG0, pll);
 
+	/* Enable hw control of SATA pll */
+	val = pll_readl(SATA_PLL_CFG0, pll);
+	val &= ~SATA_PLL_CFG0_PADPLL_RESET_SWCTL;
+	pll_writel(val, SATA_PLL_CFG0, pll);
+
 out:
 	if (pll->lock)
 		spin_unlock_irqrestore(pll->lock, flags);
-- 
1.8.1.5

[PATCH v3 5/8] clk: tegra: Add SATA clocks to Tegra124 initialization table

[Mikko Perttunen]

This adds two clocks, SATA and SATA_OOB, to the Tegra124 clock initialization
table. The clocks are needed for working SATA support.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
Already ACKed.

 drivers/clk/tegra/clk-tegra124.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/tegra/clk-tegra124.c b/drivers/clk/tegra/clk-tegra124.c
index 80efe51..6770787 100644
--- a/drivers/clk/tegra/clk-tegra124.c
+++ b/drivers/clk/tegra/clk-tegra124.c
@@ -1369,6 +1369,8 @@ static struct tegra_clk_init_table init_table[] __initdata = {
 	{TEGRA124_CLK_XUSB_HS_SRC, TEGRA124_CLK_PLL_U_60M, 60000000, 0},
 	{TEGRA124_CLK_XUSB_FALCON_SRC, TEGRA124_CLK_PLL_RE_OUT, 224000000, 0},
 	{TEGRA124_CLK_XUSB_HOST_SRC, TEGRA124_CLK_PLL_RE_OUT, 112000000, 0},
+	{TEGRA124_CLK_SATA, TEGRA124_CLK_PLL_P, 104000000, 0},
+	{TEGRA124_CLK_SATA_OOB, TEGRA124_CLK_PLL_P, 204000000, 0},
 	/* This MUST be the last entry. */
 	{TEGRA124_CLK_CLK_MAX, TEGRA124_CLK_CLK_MAX, 0, 0},
 };
-- 
1.8.1.5

[PATCH v3 6/8] ata: ahci_platform: Increase AHCI_MAX_CLKS to 4

[Mikko Perttunen]

The Tegra124 SATA controller requires 4 clocks. Increase this constant
to be able to use them all.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 drivers/ata/ahci.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 5513296..2ed84e1 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -53,7 +53,7 @@
 
 enum {
 	AHCI_MAX_PORTS		= 32,
-	AHCI_MAX_CLKS		= 3,
+	AHCI_MAX_CLKS		= 4,
 	AHCI_MAX_SG		= 168, /* hardware max is 64K */
 	AHCI_DMA_BOUNDARY	= 0xffffffff,
 	AHCI_MAX_CMDS		= 32,
-- 
1.8.1.5

[PATCH v3 7/8] ata: Add support for the Tegra124 SATA controller

[Mikko Perttunen]

This adds support for the integrated AHCI-compliant Serial ATA
controller present on the NVIDIA Tegra124 system-on-chip.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
changes since v2:
- remove fuse reading: now there is no dependency on the fuse series,
  so merging should be easier. always using the zeroth calibration
  values works for me anyway. the fuse code should be added back once
  fuses have landed, in 3.17-rc1 or 3.18.
- put calibration value variables on their own lines.
- back to using ahci_platform to manage some resources
- code style fixes

 drivers/ata/Kconfig      |   9 ++
 drivers/ata/Makefile     |   1 +
 drivers/ata/ahci_tegra.c | 406 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 416 insertions(+)
 create mode 100644 drivers/ata/ahci_tegra.c

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index b0d5b5a..e1b9278 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -142,6 +142,15 @@ config AHCI_SUNXI
 
 	  If unsure, say N.
 
+config AHCI_TEGRA
+	tristate "NVIDIA Tegra124 AHCI SATA support"
+	depends on ARCH_TEGRA
+	help
+	  This option enables support for the NVIDIA Tegra124 SoC's
+	  onboard AHCI SATA.
+
+	  If unsure, say N.
+
 config AHCI_XGENE
 	tristate "APM X-Gene 6.0Gbps AHCI SATA host controller support"
 	depends on PHY_XGENE
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 5a02aee..ae41107 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_AHCI_IMX)		+= ahci_imx.o libahci.o libahci_platform.o
 obj-$(CONFIG_AHCI_MVEBU)	+= ahci_mvebu.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
+obj-$(CONFIG_AHCI_TEGRA)	+= ahci_tegra.o libahci.o libahci_platform.o
 obj-$(CONFIG_AHCI_XGENE)	+= ahci_xgene.o libahci.o libahci_platform.o
 
 # SFF w/ custom DMA
diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c
new file mode 100644
index 0000000..d90d08c
--- /dev/null
+++ b/drivers/ata/ahci_tegra.c
@@ -0,0 +1,406 @@
+/*
+ * drivers/ata/ahci_tegra.c
+ *
+ * Copyright (c) 2014, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * Author:
+ *	Mikko Perttunen <mperttunen@nvidia.com>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/ahci_platform.h>
+#include <linux/reset.h>
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/tegra-powergate.h>
+#include <linux/regulator/consumer.h>
+#include "ahci.h"
+
+#define SATA_CONFIGURATION_0				0x180
+#define SATA_CONFIGURATION_EN_FPCI			BIT(0)
+
+#define SCFG_OFFSET					0x1000
+
+#define T_SATA0_CFG_1					0x04
+#define T_SATA0_CFG_1_IO_SPACE				BIT(0)
+#define T_SATA0_CFG_1_MEMORY_SPACE			BIT(1)
+#define T_SATA0_CFG_1_BUS_MASTER			BIT(2)
+#define T_SATA0_CFG_1_SERR				BIT(8)
+
+#define T_SATA0_CFG_9					0x24
+#define T_SATA0_CFG_9_BASE_ADDRESS_SHIFT		13
+
+#define SATA_FPCI_BAR5					0x94
+#define SATA_FPCI_BAR5_START_SHIFT			4
+
+#define SATA_INTR_MASK					0x188
+#define SATA_INTR_MASK_IP_INT_MASK			BIT(16)
+
+#define T_SATA0_AHCI_HBA_CAP_BKDR			0x300
+
+#define T_SATA0_BKDOOR_CC				0x4a4
+
+#define T_SATA0_CFG_SATA				0x54c
+#define T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN		BIT(12)
+
+#define T_SATA0_CFG_MISC				0x550
+
+#define T_SATA0_INDEX					0x680
+
+#define T_SATA0_CHX_PHY_CTRL1_GEN1			0x690
+#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK		0xff
+#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT		0
+#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK		(0xff << 8)
+#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT	8
+
+#define T_SATA0_CHX_PHY_CTRL1_GEN2			0x694
+#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_MASK		0xff
+#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_SHIFT		0
+#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_MASK		(0xff << 12)
+#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_SHIFT	12
+
+#define T_SATA0_CHX_PHY_CTRL2				0x69c
+#define T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1		0x23
+
+#define T_SATA0_CHX_PHY_CTRL11				0x6d0
+#define T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ		(0x2800 << 16)
+
+#define FUSE_SATA_CALIB					0x124
+#define FUSE_SATA_CALIB_MASK				0x3
+
+enum sata_clks {
+	AHCI_CLK_SATA		= 0,
+	AHCI_CLK_SATA_OOB	= 1,
+	AHCI_CLK_CML1		= 2,
+	AHCI_CLK_PLL_E		= 3
+};
+
+struct sata_pad_calibration {
+	u8 gen1_tx_amp;
+	u8 gen1_tx_peak;
+	u8 gen2_tx_amp;
+	u8 gen2_tx_peak;
+};
+
+static const struct sata_pad_calibration tegra124_pad_calibration[] = {
+	{0x18, 0x04, 0x18, 0x0a},
+	{0x0e, 0x04, 0x14, 0x0a},
+	{0x0e, 0x07, 0x1a, 0x0e},
+	{0x14, 0x0e, 0x1a, 0x0e},
+};
+
+struct tegra_ahci_priv {
+	struct platform_device	   *pdev;
+	void __iomem		   *sata_regs;
+	struct reset_control	   *sata_rst;
+	struct reset_control	   *sata_oob_rst;
+	struct reset_control	   *sata_cold_rst;
+	struct regulator_bulk_data supplies[5];
+};
+
+static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
+{
+	struct tegra_ahci_priv *tegra = hpriv->plat_data;
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(tegra->supplies),
+				    tegra->supplies);
+	if (ret)
+		return ret;
+
+	ret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_SATA,
+						hpriv->clks[AHCI_CLK_SATA],
+						tegra->sata_rst);
+	if (ret)
+		goto disable_regulators;
+
+	reset_control_assert(tegra->sata_oob_rst);
+	reset_control_assert(tegra->sata_cold_rst);
+
+	ret = clk_prepare_enable(hpriv->clks[AHCI_CLK_SATA_OOB]);
+	if (ret)
+		goto disable_power;
+
+	ret = clk_prepare_enable(hpriv->clks[AHCI_CLK_CML1]);
+	if (ret)
+		goto disable_oob;
+
+	ret = clk_prepare_enable(hpriv->clks[AHCI_CLK_PLL_E]);
+	if (ret)
+		goto disable_cml1;
+
+	reset_control_deassert(tegra->sata_cold_rst);
+	reset_control_deassert(tegra->sata_oob_rst);
+
+	ret = phy_power_on(hpriv->phy);
+	if (ret)
+		goto disable_plle;
+
+	return 0;
+
+disable_plle:
+	clk_disable_unprepare(hpriv->clks[AHCI_CLK_PLL_E]);
+
+disable_cml1:
+	clk_disable_unprepare(hpriv->clks[AHCI_CLK_CML1]);
+
+disable_oob:
+	clk_disable_unprepare(hpriv->clks[AHCI_CLK_SATA_OOB]);
+
+disable_power:
+	clk_disable_unprepare(hpriv->clks[AHCI_CLK_SATA]);
+
+	tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
+
+disable_regulators:
+	regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra->supplies);
+
+	return ret;
+}
+
+static void tegra_ahci_power_off(struct ahci_host_priv *hpriv)
+{
+	struct tegra_ahci_priv *tegra = hpriv->plat_data;
+
+	ahci_platform_disable_resources(hpriv);
+
+	reset_control_assert(tegra->sata_rst);
+	reset_control_assert(tegra->sata_oob_rst);
+	reset_control_assert(tegra->sata_cold_rst);
+
+	tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
+
+	regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra->supplies);
+}
+
+static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv)
+{
+	struct tegra_ahci_priv *tegra = hpriv->plat_data;
+	int ret;
+	unsigned int val;
+	struct sata_pad_calibration calib;
+
+	ret = tegra_ahci_power_on(hpriv);
+	if (ret) {
+		dev_err(&tegra->pdev->dev,
+			"failed to power on AHCI controller: %d\n", ret);
+		return ret;
+	}
+
+	val = readl(tegra->sata_regs + SATA_CONFIGURATION_0);
+	val |= SATA_CONFIGURATION_EN_FPCI;
+	writel(val, tegra->sata_regs + SATA_CONFIGURATION_0);
+
+	/* Pad calibration */
+
+	/* FIXME Always use calibration 0. Change this to read the calibration
+	 * fuse once the fuse driver has landed. */
+	val = 0;
+
+	calib = tegra124_pad_calibration[val & FUSE_SATA_CALIB_MASK];
+
+	writel(BIT(0), tegra->sata_regs + SCFG_OFFSET + T_SATA0_INDEX);
+
+	val = readl(tegra->sata_regs +
+		SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN1);
+	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK;
+	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK;
+	val |= calib.gen1_tx_amp <<
+			T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT;
+	val |= calib.gen1_tx_peak <<
+			T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT;
+	writel(val, tegra->sata_regs + SCFG_OFFSET +
+		T_SATA0_CHX_PHY_CTRL1_GEN1);
+
+	val = readl(tegra->sata_regs +
+			SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN2);
+	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_MASK;
+	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_MASK;
+	val |= calib.gen2_tx_amp <<
+			T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT;
+	val |= calib.gen2_tx_peak <<
+			T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT;
+	writel(val, tegra->sata_regs + SCFG_OFFSET +
+		T_SATA0_CHX_PHY_CTRL1_GEN2);
+
+	writel(T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ,
+		tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL11);
+	writel(T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1,
+		tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL2);
+
+	writel(0, tegra->sata_regs + SCFG_OFFSET + T_SATA0_INDEX);
+
+	/* Program controller device ID */
+
+	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
+	val |= T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN;
+	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
+
+	writel(0x01060100, tegra->sata_regs + SCFG_OFFSET + T_SATA0_BKDOOR_CC);
+
+	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
+	val &= ~T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN;
+	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
+
+	/* Enable IO & memory access, bus master mode */
+
+	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_1);
+	val |= T_SATA0_CFG_1_IO_SPACE | T_SATA0_CFG_1_MEMORY_SPACE |
+		T_SATA0_CFG_1_BUS_MASTER | T_SATA0_CFG_1_SERR;
+	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_1);
+
+	/* Program SATA MMIO */
+
+	writel(0x10000 << SATA_FPCI_BAR5_START_SHIFT,
+	       tegra->sata_regs + SATA_FPCI_BAR5);
+
+	writel(0x08000 << T_SATA0_CFG_9_BASE_ADDRESS_SHIFT,
+	       tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_9);
+
+	/* Unmask SATA interrupts */
+
+	val = readl(tegra->sata_regs + SATA_INTR_MASK);
+	val |= SATA_INTR_MASK_IP_INT_MASK;
+	writel(val, tegra->sata_regs + SATA_INTR_MASK);
+
+	return 0;
+}
+
+static void tegra_ahci_controller_deinit(struct ahci_host_priv *hpriv)
+{
+	tegra_ahci_power_off(hpriv);
+}
+
+static void tegra_ahci_host_stop(struct ata_host *host)
+{
+	struct ahci_host_priv *hpriv = host->private_data;
+
+	tegra_ahci_controller_deinit(hpriv);
+}
+
+static struct ata_port_operations ahci_tegra_port_ops = {
+	.inherits	= &ahci_ops,
+	.host_stop	= tegra_ahci_host_stop,
+};
+
+static const struct ata_port_info ahci_tegra_port_info = {
+	.flags		= AHCI_FLAG_COMMON,
+	.pio_mask	= ATA_PIO4,
+	.udma_mask	= ATA_UDMA6,
+	.port_ops	= &ahci_tegra_port_ops,
+};
+
+static const struct of_device_id tegra_ahci_of_match[] = {
+	{ .compatible = "nvidia,tegra124-ahci" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, tegra_ahci_of_match);
+
+static int tegra_ahci_probe(struct platform_device *pdev)
+{
+	struct ahci_host_priv *hpriv;
+	struct tegra_ahci_priv *tegra;
+	struct resource *res;
+	int ret;
+
+	hpriv = ahci_platform_get_resources(pdev);
+	if (IS_ERR(hpriv))
+		return PTR_ERR(hpriv);
+
+	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
+	if (!tegra)
+		return -ENOMEM;
+
+	hpriv->plat_data = tegra;
+
+	tegra->pdev = pdev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	tegra->sata_regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(tegra->sata_regs))
+		return PTR_ERR(tegra->sata_regs);
+
+	tegra->sata_rst = devm_reset_control_get(&pdev->dev, "sata");
+	if (IS_ERR(tegra->sata_rst)) {
+		dev_err(&pdev->dev, "Failed to get sata reset\n");
+		return PTR_ERR(tegra->sata_rst);
+	}
+
+	tegra->sata_oob_rst = devm_reset_control_get(&pdev->dev, "sata-oob");
+	if (IS_ERR(tegra->sata_oob_rst)) {
+		dev_err(&pdev->dev, "Failed to get sata-oob reset\n");
+		return PTR_ERR(tegra->sata_oob_rst);
+	}
+
+	tegra->sata_cold_rst = devm_reset_control_get(&pdev->dev, "sata-cold");
+	if (IS_ERR(tegra->sata_cold_rst)) {
+		dev_err(&pdev->dev, "Failed to get sata-cold reset\n");
+		return PTR_ERR(tegra->sata_cold_rst);
+	}
+
+	if (!hpriv->clks[AHCI_CLK_PLL_E])
+		return -EPROBE_DEFER;
+
+	tegra->supplies[0].supply = "avdd";
+	tegra->supplies[1].supply = "hvdd";
+	tegra->supplies[2].supply = "vddio";
+	tegra->supplies[3].supply = "target-5v";
+	tegra->supplies[4].supply = "target-12v";
+
+	ret = devm_regulator_bulk_get(&pdev->dev, ARRAY_SIZE(tegra->supplies),
+				      tegra->supplies);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to get regulators\n");
+		return ret;
+	}
+
+	ret = phy_init(hpriv->phy);
+	if (ret)
+		return ret;
+
+	ret = tegra_ahci_controller_init(hpriv);
+	if (ret)
+		goto exit_phy;
+
+	ret = ahci_platform_init_host(pdev, hpriv, &ahci_tegra_port_info,
+				      0, 0, 0);
+	if (ret)
+		goto deinit_controller;
+
+	return 0;
+
+deinit_controller:
+	tegra_ahci_controller_deinit(hpriv);
+
+exit_phy:
+	phy_exit(hpriv->phy);
+
+	return ret;
+};
+
+static struct platform_driver tegra_ahci_driver = {
+	.probe = tegra_ahci_probe,
+	.remove = ata_platform_remove_one,
+	.driver = {
+		.name = "tegra-ahci",
+		.of_match_table = tegra_ahci_of_match,
+	},
+	/* LP0 suspend support not implemented */
+};
+module_platform_driver(tegra_ahci_driver);
+
+MODULE_AUTHOR("Mikko Perttunen <mperttunen@nvidia.com>");
+MODULE_DESCRIPTION("Tegra124 AHCI SATA driver");
+MODULE_LICENSE("GPL v2");
-- 
1.8.1.5

[PATCH v3 8/8] ARM: tegra: Add options for Tegra AHCI support to tegra_defconfig

[Mikko Perttunen]

This adds ATA, SATA_AHCI and AHCI_TEGRA support to tegra_defconfig
so that the SATA support will be automatically enabled.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 arch/arm/configs/tegra_defconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig
index 285c433..2b8be48 100644
--- a/arch/arm/configs/tegra_defconfig
+++ b/arch/arm/configs/tegra_defconfig
@@ -102,6 +102,9 @@ CONFIG_BLK_DEV_SD=y
 CONFIG_BLK_DEV_SR=y
 CONFIG_SCSI_MULTI_LUN=y
 # CONFIG_SCSI_LOWLEVEL is not set
+CONFIG_ATA=y
+CONFIG_SATA_AHCI=y
+CONFIG_AHCI_TEGRA=y
 CONFIG_NETDEVICES=y
 CONFIG_DUMMY=y
 CONFIG_IGB=y
-- 
1.8.1.5

Re: [PATCH v3 1/8] of: Add NVIDIA Tegra SATA controller binding

[Varka Bhadram]

On 07/16/2014 02:24 PM, Mikko Perttunen wrote:
> This patch adds device tree binding documentation for the SATA
> controller found on NVIDIA Tegra SoCs.
>
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
>   .../devicetree/bindings/ata/tegra-sata.txt         | 30 ++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/ata/tegra-sata.txt
>
> diff --git a/Documentation/devicetree/bindings/ata/tegra-sata.txt b/Documentation/devicetree/bindings/ata/tegra-sata.txt
> new file mode 100644
> index 0000000..946f207
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ata/tegra-sata.txt
> @@ -0,0 +1,30 @@
> +Tegra124 SoC SATA AHCI controller
> +
> +Required properties :
> +- compatible : "nvidia,tegra124-ahci".
> +- reg : Should contain 2 entries:
> +  - AHCI register set (SATA BAR5)
> +  - SATA register set
> +- interrupts : Defines the interrupt used by SATA
> +- clocks : Must contain an entry for each entry in clock-names.
> +  See ../clocks/clock-bindings.txt for details.
> +- clock-names : Must include the following entries:
> +  - sata
> +  - sata-oob
> +  - cml1
> +  - pll_e

It would be more readable if it like this...

Required properties :
- compatible	: "nvidia,tegra124-ahci".
- reg		: Should contain 2 entries:
		  - AHCI register set (SATA BAR5)
		  - SATA register set
- interrupts	: Defines the interrupt used by SATA
- clocks	: Must contain an entry for each entry in clock-names.
		  See ../clocks/clock-bindings.txt for details.
....

> +- resets : Must contain an entry for each entry in reset-names.
> +  See ../reset/reset.txt for details.
> +- reset-names : Must include the following entries:
> +  - sata
> +  - sata-oob
> +  - sata-cold
> +- phys : Must contain an entry for each entry in phy-names.
> +  See ../phy/phy-bindings.txt for details.
> +- phy-names : Must include the following entries:
> +  - sata-phy : XUSB PADCTL SATA PHY
> +- hvdd-supply : Defines the SATA HVDD regulator
> +- vddio-supply : Defines the SATA VDDIO regulator
> +- avdd-supply : Defines the SATA AVDD regulator
> +- target-5v-supply : Defines the SATA 5V power regulator
> +- target-12v-supply : Defines the SATA 12V power regulator

Same....


-- 
Regards,
Varka Bhadram.

Re: [PATCH v3 7/8] ata: Add support for the Tegra124 SATA controller

[Hans de Goede]

Hi,

Thanks for making the changes to use more libahci_platform
functionality. Overall this looks good, I've a few small remarks
inline.

On 07/16/2014 10:54 AM, Mikko Perttunen wrote:
> This adds support for the integrated AHCI-compliant Serial ATA
> controller present on the NVIDIA Tegra124 system-on-chip.
> 
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
> changes since v2:
> - remove fuse reading: now there is no dependency on the fuse series,
>   so merging should be easier. always using the zeroth calibration
>   values works for me anyway. the fuse code should be added back once
>   fuses have landed, in 3.17-rc1 or 3.18.
> - put calibration value variables on their own lines.
> - back to using ahci_platform to manage some resources
> - code style fixes
> 
>  drivers/ata/Kconfig      |   9 ++
>  drivers/ata/Makefile     |   1 +
>  drivers/ata/ahci_tegra.c | 406 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 416 insertions(+)
>  create mode 100644 drivers/ata/ahci_tegra.c
> 
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index b0d5b5a..e1b9278 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -142,6 +142,15 @@ config AHCI_SUNXI
>  
>  	  If unsure, say N.
>  
> +config AHCI_TEGRA
> +	tristate "NVIDIA Tegra124 AHCI SATA support"
> +	depends on ARCH_TEGRA
> +	help
> +	  This option enables support for the NVIDIA Tegra124 SoC's
> +	  onboard AHCI SATA.
> +
> +	  If unsure, say N.
> +
>  config AHCI_XGENE
>  	tristate "APM X-Gene 6.0Gbps AHCI SATA host controller support"
>  	depends on PHY_XGENE
> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> index 5a02aee..ae41107 100644
> --- a/drivers/ata/Makefile
> +++ b/drivers/ata/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_AHCI_IMX)		+= ahci_imx.o libahci.o libahci_platform.o
>  obj-$(CONFIG_AHCI_MVEBU)	+= ahci_mvebu.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
> +obj-$(CONFIG_AHCI_TEGRA)	+= ahci_tegra.o libahci.o libahci_platform.o
>  obj-$(CONFIG_AHCI_XGENE)	+= ahci_xgene.o libahci.o libahci_platform.o
>  
>  # SFF w/ custom DMA
> diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c
> new file mode 100644
> index 0000000..d90d08c
> --- /dev/null
> +++ b/drivers/ata/ahci_tegra.c
> @@ -0,0 +1,406 @@
> +/*
> + * drivers/ata/ahci_tegra.c
> + *
> + * Copyright (c) 2014, NVIDIA CORPORATION.  All rights reserved.
> + *
> + * Author:
> + *	Mikko Perttunen <mperttunen@nvidia.com>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/ahci_platform.h>
> +#include <linux/reset.h>
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/tegra-powergate.h>
> +#include <linux/regulator/consumer.h>
> +#include "ahci.h"
> +
> +#define SATA_CONFIGURATION_0				0x180
> +#define SATA_CONFIGURATION_EN_FPCI			BIT(0)
> +
> +#define SCFG_OFFSET					0x1000
> +
> +#define T_SATA0_CFG_1					0x04
> +#define T_SATA0_CFG_1_IO_SPACE				BIT(0)
> +#define T_SATA0_CFG_1_MEMORY_SPACE			BIT(1)
> +#define T_SATA0_CFG_1_BUS_MASTER			BIT(2)
> +#define T_SATA0_CFG_1_SERR				BIT(8)
> +
> +#define T_SATA0_CFG_9					0x24
> +#define T_SATA0_CFG_9_BASE_ADDRESS_SHIFT		13
> +
> +#define SATA_FPCI_BAR5					0x94
> +#define SATA_FPCI_BAR5_START_SHIFT			4
> +
> +#define SATA_INTR_MASK					0x188
> +#define SATA_INTR_MASK_IP_INT_MASK			BIT(16)
> +
> +#define T_SATA0_AHCI_HBA_CAP_BKDR			0x300
> +
> +#define T_SATA0_BKDOOR_CC				0x4a4
> +
> +#define T_SATA0_CFG_SATA				0x54c
> +#define T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN		BIT(12)
> +
> +#define T_SATA0_CFG_MISC				0x550
> +
> +#define T_SATA0_INDEX					0x680
> +
> +#define T_SATA0_CHX_PHY_CTRL1_GEN1			0x690
> +#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK		0xff
> +#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT		0
> +#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK		(0xff << 8)
> +#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT	8
> +
> +#define T_SATA0_CHX_PHY_CTRL1_GEN2			0x694
> +#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_MASK		0xff
> +#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_SHIFT		0
> +#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_MASK		(0xff << 12)
> +#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_SHIFT	12
> +
> +#define T_SATA0_CHX_PHY_CTRL2				0x69c
> +#define T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1		0x23
> +
> +#define T_SATA0_CHX_PHY_CTRL11				0x6d0
> +#define T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ		(0x2800 << 16)
> +
> +#define FUSE_SATA_CALIB					0x124
> +#define FUSE_SATA_CALIB_MASK				0x3
> +
> +enum sata_clks {
> +	AHCI_CLK_SATA		= 0,
> +	AHCI_CLK_SATA_OOB	= 1,
> +	AHCI_CLK_CML1		= 2,
> +	AHCI_CLK_PLL_E		= 3
> +};

You should mention that having the clocks in this order is mandatory
in Documentation/devicetree/bindings/ata/tegra-sata.txt / in
"[PATCH v3 1/8] of: Add NVIDIA Tegra SATA controller binding"


> +
> +struct sata_pad_calibration {
> +	u8 gen1_tx_amp;
> +	u8 gen1_tx_peak;
> +	u8 gen2_tx_amp;
> +	u8 gen2_tx_peak;
> +};
> +
> +static const struct sata_pad_calibration tegra124_pad_calibration[] = {
> +	{0x18, 0x04, 0x18, 0x0a},
> +	{0x0e, 0x04, 0x14, 0x0a},
> +	{0x0e, 0x07, 0x1a, 0x0e},
> +	{0x14, 0x0e, 0x1a, 0x0e},
> +};
> +
> +struct tegra_ahci_priv {
> +	struct platform_device	   *pdev;
> +	void __iomem		   *sata_regs;
> +	struct reset_control	   *sata_rst;
> +	struct reset_control	   *sata_oob_rst;
> +	struct reset_control	   *sata_cold_rst;
> +	struct regulator_bulk_data supplies[5];
> +};
> +
> +static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
> +{
> +	struct tegra_ahci_priv *tegra = hpriv->plat_data;
> +	int ret;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(tegra->supplies),
> +				    tegra->supplies);
> +	if (ret)
> +		return ret;
> +
> +	ret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_SATA,
> +						hpriv->clks[AHCI_CLK_SATA],
> +						tegra->sata_rst);
> +	if (ret)
> +		goto disable_regulators;
> +
> +	reset_control_assert(tegra->sata_oob_rst);
> +	reset_control_assert(tegra->sata_cold_rst);
> +
> +	ret = clk_prepare_enable(hpriv->clks[AHCI_CLK_SATA_OOB]);
> +	if (ret)
> +		goto disable_power;
> +
> +	ret = clk_prepare_enable(hpriv->clks[AHCI_CLK_CML1]);
> +	if (ret)
> +		goto disable_oob;
> +
> +	ret = clk_prepare_enable(hpriv->clks[AHCI_CLK_PLL_E]);
> +	if (ret)
> +		goto disable_cml1;
> +
> +	reset_control_deassert(tegra->sata_cold_rst);
> +	reset_control_deassert(tegra->sata_oob_rst);
> +
> +	ret = phy_power_on(hpriv->phy);
> +	if (ret)
> +		goto disable_plle;
> +
> +	return 0;
> +
> +disable_plle:
> +	clk_disable_unprepare(hpriv->clks[AHCI_CLK_PLL_E]);
> +
> +disable_cml1:
> +	clk_disable_unprepare(hpriv->clks[AHCI_CLK_CML1]);
> +
> +disable_oob:
> +	clk_disable_unprepare(hpriv->clks[AHCI_CLK_SATA_OOB]);
> +
> +disable_power:
> +	clk_disable_unprepare(hpriv->clks[AHCI_CLK_SATA]);
> +
> +	tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
> +
> +disable_regulators:
> +	regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra->supplies);
> +
> +	return ret;
> +}
> +
> +static void tegra_ahci_power_off(struct ahci_host_priv *hpriv)
> +{
> +	struct tegra_ahci_priv *tegra = hpriv->plat_data;
> +
> +	ahci_platform_disable_resources(hpriv);

Disable resources does a phy_exit, unless this is a nop with your
phy driver, you must re-do the phy_init in tegra_ahci_power_on()
arguably the phy_init / phy_exit in libahci_platform should be
moved to hci_platform_get_resources and ahci_platform_put_resources,
if you decide to go for that please make sure you coordinate with
other libahci_platform users which use a phy (see the dts files).

> +
> +	reset_control_assert(tegra->sata_rst);
> +	reset_control_assert(tegra->sata_oob_rst);
> +	reset_control_assert(tegra->sata_cold_rst);
> +
> +	tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
> +
> +	regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra->supplies);
> +}
> +
> +static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv)
> +{
> +	struct tegra_ahci_priv *tegra = hpriv->plat_data;
> +	int ret;
> +	unsigned int val;
> +	struct sata_pad_calibration calib;
> +
> +	ret = tegra_ahci_power_on(hpriv);
> +	if (ret) {
> +		dev_err(&tegra->pdev->dev,
> +			"failed to power on AHCI controller: %d\n", ret);
> +		return ret;
> +	}
> +
> +	val = readl(tegra->sata_regs + SATA_CONFIGURATION_0);
> +	val |= SATA_CONFIGURATION_EN_FPCI;
> +	writel(val, tegra->sata_regs + SATA_CONFIGURATION_0);
> +
> +	/* Pad calibration */
> +
> +	/* FIXME Always use calibration 0. Change this to read the calibration
> +	 * fuse once the fuse driver has landed. */
> +	val = 0;
> +
> +	calib = tegra124_pad_calibration[val & FUSE_SATA_CALIB_MASK];
> +
> +	writel(BIT(0), tegra->sata_regs + SCFG_OFFSET + T_SATA0_INDEX);
> +
> +	val = readl(tegra->sata_regs +
> +		SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN1);
> +	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK;
> +	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK;
> +	val |= calib.gen1_tx_amp <<
> +			T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT;
> +	val |= calib.gen1_tx_peak <<
> +			T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT;
> +	writel(val, tegra->sata_regs + SCFG_OFFSET +
> +		T_SATA0_CHX_PHY_CTRL1_GEN1);
> +
> +	val = readl(tegra->sata_regs +
> +			SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN2);
> +	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_MASK;
> +	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_MASK;
> +	val |= calib.gen2_tx_amp <<
> +			T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT;
> +	val |= calib.gen2_tx_peak <<
> +			T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT;
> +	writel(val, tegra->sata_regs + SCFG_OFFSET +
> +		T_SATA0_CHX_PHY_CTRL1_GEN2);
> +
> +	writel(T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ,
> +		tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL11);
> +	writel(T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1,
> +		tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL2);
> +
> +	writel(0, tegra->sata_regs + SCFG_OFFSET + T_SATA0_INDEX);
> +
> +	/* Program controller device ID */
> +
> +	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
> +	val |= T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN;
> +	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
> +
> +	writel(0x01060100, tegra->sata_regs + SCFG_OFFSET + T_SATA0_BKDOOR_CC);
> +
> +	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
> +	val &= ~T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN;
> +	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
> +
> +	/* Enable IO & memory access, bus master mode */
> +
> +	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_1);
> +	val |= T_SATA0_CFG_1_IO_SPACE | T_SATA0_CFG_1_MEMORY_SPACE |
> +		T_SATA0_CFG_1_BUS_MASTER | T_SATA0_CFG_1_SERR;
> +	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_1);
> +
> +	/* Program SATA MMIO */
> +
> +	writel(0x10000 << SATA_FPCI_BAR5_START_SHIFT,
> +	       tegra->sata_regs + SATA_FPCI_BAR5);
> +
> +	writel(0x08000 << T_SATA0_CFG_9_BASE_ADDRESS_SHIFT,
> +	       tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_9);
> +
> +	/* Unmask SATA interrupts */
> +
> +	val = readl(tegra->sata_regs + SATA_INTR_MASK);
> +	val |= SATA_INTR_MASK_IP_INT_MASK;
> +	writel(val, tegra->sata_regs + SATA_INTR_MASK);
> +
> +	return 0;
> +}
> +
> +static void tegra_ahci_controller_deinit(struct ahci_host_priv *hpriv)
> +{
> +	tegra_ahci_power_off(hpriv);
> +}
> +
> +static void tegra_ahci_host_stop(struct ata_host *host)
> +{
> +	struct ahci_host_priv *hpriv = host->private_data;
> +
> +	tegra_ahci_controller_deinit(hpriv);
> +}
> +
> +static struct ata_port_operations ahci_tegra_port_ops = {
> +	.inherits	= &ahci_ops,
> +	.host_stop	= tegra_ahci_host_stop,
> +};
> +
> +static const struct ata_port_info ahci_tegra_port_info = {
> +	.flags		= AHCI_FLAG_COMMON,
> +	.pio_mask	= ATA_PIO4,
> +	.udma_mask	= ATA_UDMA6,
> +	.port_ops	= &ahci_tegra_port_ops,
> +};
> +
> +static const struct of_device_id tegra_ahci_of_match[] = {
> +	{ .compatible = "nvidia,tegra124-ahci" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, tegra_ahci_of_match);
> +
> +static int tegra_ahci_probe(struct platform_device *pdev)
> +{
> +	struct ahci_host_priv *hpriv;
> +	struct tegra_ahci_priv *tegra;
> +	struct resource *res;
> +	int ret;
> +
> +	hpriv = ahci_platform_get_resources(pdev);
> +	if (IS_ERR(hpriv))
> +		return PTR_ERR(hpriv);
> +
> +	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
> +	if (!tegra)
> +		return -ENOMEM;
> +
> +	hpriv->plat_data = tegra;
> +
> +	tegra->pdev = pdev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	tegra->sata_regs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(tegra->sata_regs))
> +		return PTR_ERR(tegra->sata_regs);
> +
> +	tegra->sata_rst = devm_reset_control_get(&pdev->dev, "sata");
> +	if (IS_ERR(tegra->sata_rst)) {
> +		dev_err(&pdev->dev, "Failed to get sata reset\n");
> +		return PTR_ERR(tegra->sata_rst);
> +	}
> +
> +	tegra->sata_oob_rst = devm_reset_control_get(&pdev->dev, "sata-oob");
> +	if (IS_ERR(tegra->sata_oob_rst)) {
> +		dev_err(&pdev->dev, "Failed to get sata-oob reset\n");
> +		return PTR_ERR(tegra->sata_oob_rst);
> +	}
> +
> +	tegra->sata_cold_rst = devm_reset_control_get(&pdev->dev, "sata-cold");
> +	if (IS_ERR(tegra->sata_cold_rst)) {
> +		dev_err(&pdev->dev, "Failed to get sata-cold reset\n");
> +		return PTR_ERR(tegra->sata_cold_rst);
> +	}
> +
> +	if (!hpriv->clks[AHCI_CLK_PLL_E])
> +		return -EPROBE_DEFER;
> +
> +	tegra->supplies[0].supply = "avdd";
> +	tegra->supplies[1].supply = "hvdd";
> +	tegra->supplies[2].supply = "vddio";
> +	tegra->supplies[3].supply = "target-5v";
> +	tegra->supplies[4].supply = "target-12v";
> +
> +	ret = devm_regulator_bulk_get(&pdev->dev, ARRAY_SIZE(tegra->supplies),
> +				      tegra->supplies);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to get regulators\n");
> +		return ret;
> +	}
> +
> +	ret = phy_init(hpriv->phy);
> +	if (ret)
> +		return ret;
> +
> +	ret = tegra_ahci_controller_init(hpriv);
> +	if (ret)
> +		goto exit_phy;
> +
> +	ret = ahci_platform_init_host(pdev, hpriv, &ahci_tegra_port_info,
> +				      0, 0, 0);
> +	if (ret)
> +		goto deinit_controller;
> +
> +	return 0;
> +
> +deinit_controller:
> +	tegra_ahci_controller_deinit(hpriv);
> +
> +exit_phy:
> +	phy_exit(hpriv->phy);
> +
> +	return ret;
> +};
> +
> +static struct platform_driver tegra_ahci_driver = {
> +	.probe = tegra_ahci_probe,
> +	.remove = ata_platform_remove_one,
> +	.driver = {
> +		.name = "tegra-ahci",
> +		.of_match_table = tegra_ahci_of_match,
> +	},
> +	/* LP0 suspend support not implemented */
> +};
> +module_platform_driver(tegra_ahci_driver);
> +
> +MODULE_AUTHOR("Mikko Perttunen <mperttunen@nvidia.com>");
> +MODULE_DESCRIPTION("Tegra124 AHCI SATA driver");
> +MODULE_LICENSE("GPL v2");
> 

Thanks & Regards,

Hans

[PATCH v4 1/8] of: Add NVIDIA Tegra SATA controller binding

[Mikko Perttunen]

This patch adds device tree binding documentation for the SATA
controller found on NVIDIA Tegra SoCs.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
v4: clarify mandatory clock order

 .../devicetree/bindings/ata/tegra-sata.txt         | 31 ++++++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ata/tegra-sata.txt

diff --git a/Documentation/devicetree/bindings/ata/tegra-sata.txt b/Documentation/devicetree/bindings/ata/tegra-sata.txt
new file mode 100644
index 0000000..de2f775
--- /dev/null
+++ b/Documentation/devicetree/bindings/ata/tegra-sata.txt
@@ -0,0 +1,31 @@
+Tegra124 SoC SATA AHCI controller
+
+Required properties :
+- compatible : "nvidia,tegra124-ahci".
+- reg : Should contain 2 entries:
+  - AHCI register set (SATA BAR5)
+  - SATA register set
+- interrupts : Defines the interrupt used by SATA
+- clocks : Must contain an entry for each entry in clock-names.
+  See ../clocks/clock-bindings.txt for details.
+- clock-names : Must include the following entries in the same
+  order as listed here:
+  - sata
+  - sata-oob
+  - cml1
+  - pll_e
+- resets : Must contain an entry for each entry in reset-names.
+  See ../reset/reset.txt for details.
+- reset-names : Must include the following entries:
+  - sata
+  - sata-oob
+  - sata-cold
+- phys : Must contain an entry for each entry in phy-names.
+  See ../phy/phy-bindings.txt for details.
+- phy-names : Must include the following entries:
+  - sata-phy : XUSB PADCTL SATA PHY
+- hvdd-supply : Defines the SATA HVDD regulator
+- vddio-supply : Defines the SATA VDDIO regulator
+- avdd-supply : Defines the SATA AVDD regulator
+- target-5v-supply : Defines the SATA 5V power regulator
+- target-12v-supply : Defines the SATA 12V power regulator
-- 
1.8.1.5

[PATCH v4 7/8] ata: Add support for the Tegra124 SATA controller

[Mikko Perttunen]

This adds support for the integrated AHCI-compliant Serial ATA
controller present on the NVIDIA Tegra124 system-on-chip.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
v4: init phy in power_on

 drivers/ata/Kconfig      |   9 ++
 drivers/ata/Makefile     |   1 +
 drivers/ata/ahci_tegra.c | 406 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 416 insertions(+)
 create mode 100644 drivers/ata/ahci_tegra.c

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index b0d5b5a..e1b9278 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -142,6 +142,15 @@ config AHCI_SUNXI
 
 	  If unsure, say N.
 
+config AHCI_TEGRA
+	tristate "NVIDIA Tegra124 AHCI SATA support"
+	depends on ARCH_TEGRA
+	help
+	  This option enables support for the NVIDIA Tegra124 SoC's
+	  onboard AHCI SATA.
+
+	  If unsure, say N.
+
 config AHCI_XGENE
 	tristate "APM X-Gene 6.0Gbps AHCI SATA host controller support"
 	depends on PHY_XGENE
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 5a02aee..ae41107 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_AHCI_IMX)		+= ahci_imx.o libahci.o libahci_platform.o
 obj-$(CONFIG_AHCI_MVEBU)	+= ahci_mvebu.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
+obj-$(CONFIG_AHCI_TEGRA)	+= ahci_tegra.o libahci.o libahci_platform.o
 obj-$(CONFIG_AHCI_XGENE)	+= ahci_xgene.o libahci.o libahci_platform.o
 
 # SFF w/ custom DMA
diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c
new file mode 100644
index 0000000..0f59ae4
--- /dev/null
+++ b/drivers/ata/ahci_tegra.c
@@ -0,0 +1,406 @@
+/*
+ * drivers/ata/ahci_tegra.c
+ *
+ * Copyright (c) 2014, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * Author:
+ *	Mikko Perttunen <mperttunen@nvidia.com>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/ahci_platform.h>
+#include <linux/reset.h>
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/tegra-powergate.h>
+#include <linux/regulator/consumer.h>
+#include "ahci.h"
+
+#define SATA_CONFIGURATION_0				0x180
+#define SATA_CONFIGURATION_EN_FPCI			BIT(0)
+
+#define SCFG_OFFSET					0x1000
+
+#define T_SATA0_CFG_1					0x04
+#define T_SATA0_CFG_1_IO_SPACE				BIT(0)
+#define T_SATA0_CFG_1_MEMORY_SPACE			BIT(1)
+#define T_SATA0_CFG_1_BUS_MASTER			BIT(2)
+#define T_SATA0_CFG_1_SERR				BIT(8)
+
+#define T_SATA0_CFG_9					0x24
+#define T_SATA0_CFG_9_BASE_ADDRESS_SHIFT		13
+
+#define SATA_FPCI_BAR5					0x94
+#define SATA_FPCI_BAR5_START_SHIFT			4
+
+#define SATA_INTR_MASK					0x188
+#define SATA_INTR_MASK_IP_INT_MASK			BIT(16)
+
+#define T_SATA0_AHCI_HBA_CAP_BKDR			0x300
+
+#define T_SATA0_BKDOOR_CC				0x4a4
+
+#define T_SATA0_CFG_SATA				0x54c
+#define T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN		BIT(12)
+
+#define T_SATA0_CFG_MISC				0x550
+
+#define T_SATA0_INDEX					0x680
+
+#define T_SATA0_CHX_PHY_CTRL1_GEN1			0x690
+#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK		0xff
+#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT		0
+#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK		(0xff << 8)
+#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT	8
+
+#define T_SATA0_CHX_PHY_CTRL1_GEN2			0x694
+#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_MASK		0xff
+#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_SHIFT		0
+#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_MASK		(0xff << 12)
+#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_SHIFT	12
+
+#define T_SATA0_CHX_PHY_CTRL2				0x69c
+#define T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1		0x23
+
+#define T_SATA0_CHX_PHY_CTRL11				0x6d0
+#define T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ		(0x2800 << 16)
+
+#define FUSE_SATA_CALIB					0x124
+#define FUSE_SATA_CALIB_MASK				0x3
+
+enum sata_clks {
+	AHCI_CLK_SATA		= 0,
+	AHCI_CLK_SATA_OOB	= 1,
+	AHCI_CLK_CML1		= 2,
+	AHCI_CLK_PLL_E		= 3
+};
+
+struct sata_pad_calibration {
+	u8 gen1_tx_amp;
+	u8 gen1_tx_peak;
+	u8 gen2_tx_amp;
+	u8 gen2_tx_peak;
+};
+
+static const struct sata_pad_calibration tegra124_pad_calibration[] = {
+	{0x18, 0x04, 0x18, 0x0a},
+	{0x0e, 0x04, 0x14, 0x0a},
+	{0x0e, 0x07, 0x1a, 0x0e},
+	{0x14, 0x0e, 0x1a, 0x0e},
+};
+
+struct tegra_ahci_priv {
+	struct platform_device	   *pdev;
+	void __iomem		   *sata_regs;
+	struct reset_control	   *sata_rst;
+	struct reset_control	   *sata_oob_rst;
+	struct reset_control	   *sata_cold_rst;
+	struct regulator_bulk_data supplies[5];
+};
+
+static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
+{
+	struct tegra_ahci_priv *tegra = hpriv->plat_data;
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(tegra->supplies),
+				    tegra->supplies);
+	if (ret)
+		return ret;
+
+	ret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_SATA,
+						hpriv->clks[AHCI_CLK_SATA],
+						tegra->sata_rst);
+	if (ret)
+		goto disable_regulators;
+
+	reset_control_assert(tegra->sata_oob_rst);
+	reset_control_assert(tegra->sata_cold_rst);
+
+	ret = clk_prepare_enable(hpriv->clks[AHCI_CLK_SATA_OOB]);
+	if (ret)
+		goto disable_power;
+
+	ret = clk_prepare_enable(hpriv->clks[AHCI_CLK_CML1]);
+	if (ret)
+		goto disable_oob;
+
+	ret = clk_prepare_enable(hpriv->clks[AHCI_CLK_PLL_E]);
+	if (ret)
+		goto disable_cml1;
+
+	reset_control_deassert(tegra->sata_cold_rst);
+	reset_control_deassert(tegra->sata_oob_rst);
+
+	ret = phy_init(hpriv->phy);
+	if (ret)
+		goto disable_plle;
+
+	ret = phy_power_on(hpriv->phy);
+	if (ret)
+		goto exit_phy;
+
+	return 0;
+
+exit_phy:
+	phy_exit(hpriv->phy);
+
+disable_plle:
+	clk_disable_unprepare(hpriv->clks[AHCI_CLK_PLL_E]);
+
+disable_cml1:
+	clk_disable_unprepare(hpriv->clks[AHCI_CLK_CML1]);
+
+disable_oob:
+	clk_disable_unprepare(hpriv->clks[AHCI_CLK_SATA_OOB]);
+
+disable_power:
+	clk_disable_unprepare(hpriv->clks[AHCI_CLK_SATA]);
+
+	tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
+
+disable_regulators:
+	regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra->supplies);
+
+	return ret;
+}
+
+static void tegra_ahci_power_off(struct ahci_host_priv *hpriv)
+{
+	struct tegra_ahci_priv *tegra = hpriv->plat_data;
+
+	ahci_platform_disable_resources(hpriv);
+
+	reset_control_assert(tegra->sata_rst);
+	reset_control_assert(tegra->sata_oob_rst);
+	reset_control_assert(tegra->sata_cold_rst);
+
+	tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
+
+	regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra->supplies);
+}
+
+static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv)
+{
+	struct tegra_ahci_priv *tegra = hpriv->plat_data;
+	int ret;
+	unsigned int val;
+	struct sata_pad_calibration calib;
+
+	ret = tegra_ahci_power_on(hpriv);
+	if (ret) {
+		dev_err(&tegra->pdev->dev,
+			"failed to power on AHCI controller: %d\n", ret);
+		return ret;
+	}
+
+	val = readl(tegra->sata_regs + SATA_CONFIGURATION_0);
+	val |= SATA_CONFIGURATION_EN_FPCI;
+	writel(val, tegra->sata_regs + SATA_CONFIGURATION_0);
+
+	/* Pad calibration */
+
+	/* FIXME Always use calibration 0. Change this to read the calibration
+	 * fuse once the fuse driver has landed. */
+	val = 0;
+
+	calib = tegra124_pad_calibration[val & FUSE_SATA_CALIB_MASK];
+
+	writel(BIT(0), tegra->sata_regs + SCFG_OFFSET + T_SATA0_INDEX);
+
+	val = readl(tegra->sata_regs +
+		SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN1);
+	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK;
+	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK;
+	val |= calib.gen1_tx_amp <<
+			T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT;
+	val |= calib.gen1_tx_peak <<
+			T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT;
+	writel(val, tegra->sata_regs + SCFG_OFFSET +
+		T_SATA0_CHX_PHY_CTRL1_GEN1);
+
+	val = readl(tegra->sata_regs +
+			SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN2);
+	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_MASK;
+	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_MASK;
+	val |= calib.gen2_tx_amp <<
+			T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT;
+	val |= calib.gen2_tx_peak <<
+			T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT;
+	writel(val, tegra->sata_regs + SCFG_OFFSET +
+		T_SATA0_CHX_PHY_CTRL1_GEN2);
+
+	writel(T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ,
+		tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL11);
+	writel(T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1,
+		tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL2);
+
+	writel(0, tegra->sata_regs + SCFG_OFFSET + T_SATA0_INDEX);
+
+	/* Program controller device ID */
+
+	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
+	val |= T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN;
+	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
+
+	writel(0x01060100, tegra->sata_regs + SCFG_OFFSET + T_SATA0_BKDOOR_CC);
+
+	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
+	val &= ~T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN;
+	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
+
+	/* Enable IO & memory access, bus master mode */
+
+	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_1);
+	val |= T_SATA0_CFG_1_IO_SPACE | T_SATA0_CFG_1_MEMORY_SPACE |
+		T_SATA0_CFG_1_BUS_MASTER | T_SATA0_CFG_1_SERR;
+	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_1);
+
+	/* Program SATA MMIO */
+
+	writel(0x10000 << SATA_FPCI_BAR5_START_SHIFT,
+	       tegra->sata_regs + SATA_FPCI_BAR5);
+
+	writel(0x08000 << T_SATA0_CFG_9_BASE_ADDRESS_SHIFT,
+	       tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_9);
+
+	/* Unmask SATA interrupts */
+
+	val = readl(tegra->sata_regs + SATA_INTR_MASK);
+	val |= SATA_INTR_MASK_IP_INT_MASK;
+	writel(val, tegra->sata_regs + SATA_INTR_MASK);
+
+	return 0;
+}
+
+static void tegra_ahci_controller_deinit(struct ahci_host_priv *hpriv)
+{
+	tegra_ahci_power_off(hpriv);
+}
+
+static void tegra_ahci_host_stop(struct ata_host *host)
+{
+	struct ahci_host_priv *hpriv = host->private_data;
+
+	tegra_ahci_controller_deinit(hpriv);
+}
+
+static struct ata_port_operations ahci_tegra_port_ops = {
+	.inherits	= &ahci_ops,
+	.host_stop	= tegra_ahci_host_stop,
+};
+
+static const struct ata_port_info ahci_tegra_port_info = {
+	.flags		= AHCI_FLAG_COMMON,
+	.pio_mask	= ATA_PIO4,
+	.udma_mask	= ATA_UDMA6,
+	.port_ops	= &ahci_tegra_port_ops,
+};
+
+static const struct of_device_id tegra_ahci_of_match[] = {
+	{ .compatible = "nvidia,tegra124-ahci" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, tegra_ahci_of_match);
+
+static int tegra_ahci_probe(struct platform_device *pdev)
+{
+	struct ahci_host_priv *hpriv;
+	struct tegra_ahci_priv *tegra;
+	struct resource *res;
+	int ret;
+
+	hpriv = ahci_platform_get_resources(pdev);
+	if (IS_ERR(hpriv))
+		return PTR_ERR(hpriv);
+
+	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
+	if (!tegra)
+		return -ENOMEM;
+
+	hpriv->plat_data = tegra;
+
+	tegra->pdev = pdev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	tegra->sata_regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(tegra->sata_regs))
+		return PTR_ERR(tegra->sata_regs);
+
+	tegra->sata_rst = devm_reset_control_get(&pdev->dev, "sata");
+	if (IS_ERR(tegra->sata_rst)) {
+		dev_err(&pdev->dev, "Failed to get sata reset\n");
+		return PTR_ERR(tegra->sata_rst);
+	}
+
+	tegra->sata_oob_rst = devm_reset_control_get(&pdev->dev, "sata-oob");
+	if (IS_ERR(tegra->sata_oob_rst)) {
+		dev_err(&pdev->dev, "Failed to get sata-oob reset\n");
+		return PTR_ERR(tegra->sata_oob_rst);
+	}
+
+	tegra->sata_cold_rst = devm_reset_control_get(&pdev->dev, "sata-cold");
+	if (IS_ERR(tegra->sata_cold_rst)) {
+		dev_err(&pdev->dev, "Failed to get sata-cold reset\n");
+		return PTR_ERR(tegra->sata_cold_rst);
+	}
+
+	if (!hpriv->clks[AHCI_CLK_PLL_E])
+		return -EPROBE_DEFER;
+
+	tegra->supplies[0].supply = "avdd";
+	tegra->supplies[1].supply = "hvdd";
+	tegra->supplies[2].supply = "vddio";
+	tegra->supplies[3].supply = "target-5v";
+	tegra->supplies[4].supply = "target-12v";
+
+	ret = devm_regulator_bulk_get(&pdev->dev, ARRAY_SIZE(tegra->supplies),
+				      tegra->supplies);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to get regulators\n");
+		return ret;
+	}
+
+	ret = tegra_ahci_controller_init(hpriv);
+	if (ret)
+		return ret;
+
+	ret = ahci_platform_init_host(pdev, hpriv, &ahci_tegra_port_info,
+				      0, 0, 0);
+	if (ret)
+		goto deinit_controller;
+
+	return 0;
+
+deinit_controller:
+	tegra_ahci_controller_deinit(hpriv);
+
+	return ret;
+};
+
+static struct platform_driver tegra_ahci_driver = {
+	.probe = tegra_ahci_probe,
+	.remove = ata_platform_remove_one,
+	.driver = {
+		.name = "tegra-ahci",
+		.of_match_table = tegra_ahci_of_match,
+	},
+	/* LP0 suspend support not implemented */
+};
+module_platform_driver(tegra_ahci_driver);
+
+MODULE_AUTHOR("Mikko Perttunen <mperttunen@nvidia.com>");
+MODULE_DESCRIPTION("Tegra124 AHCI SATA driver");
+MODULE_LICENSE("GPL v2");
-- 
1.8.1.5

Re: [PATCH v3 7/8] ata: Add support for the Tegra124 SATA controller

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1237 bytes --]

On Wed, Jul 16, 2014 at 01:14:30PM +0200, Hans de Goede wrote:
> On 07/16/2014 10:54 AM, Mikko Perttunen wrote:
[...]
> > +enum sata_clks {
> > +	AHCI_CLK_SATA		= 0,
> > +	AHCI_CLK_SATA_OOB	= 1,
> > +	AHCI_CLK_CML1		= 2,
> > +	AHCI_CLK_PLL_E		= 3
> > +};
> 
> You should mention that having the clocks in this order is mandatory
> in Documentation/devicetree/bindings/ata/tegra-sata.txt / in
> "[PATCH v3 1/8] of: Add NVIDIA Tegra SATA controller binding"

This is precisely why I said that using a library for resources is a bad
idea. Why on earth would we want to mandate a specific order? The clock
bindings specifically have the clock-names so that the order in which
the clocks are listed in the clocks property doesn't matter. Now because
of these library helpers we can no longer do that, but instead need to
fix the order.

So we're now in a position where a Linux-specific driver, worse
actually, a Linux helper library dictates what the device tree binding
must look like. This obviously conflicts with what device tree people
have been recommending for years.

What this also means is that since the DT bindings are ABI, the libahci
platform library code effectively becomes an ABI too.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 1/8] of: Add NVIDIA Tegra SATA controller binding

[Mikko Perttunen]

Thanks for you comment. I decided to keep the format as is for now, 
however, to keep a consistent style between all binding files describing 
Tegra hardware.

Mikko

On 16/07/14 12:26, Varka Bhadram wrote:
> On 07/16/2014 02:24 PM, Mikko Perttunen wrote:
>> This patch adds device tree binding documentation for the SATA
>> controller found on NVIDIA Tegra SoCs.
>>
>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>> ---
>>    .../devicetree/bindings/ata/tegra-sata.txt         | 30 ++++++++++++++++++++++
>>    1 file changed, 30 insertions(+)
>>    create mode 100644 Documentation/devicetree/bindings/ata/tegra-sata.txt
>>
>> diff --git a/Documentation/devicetree/bindings/ata/tegra-sata.txt b/Documentation/devicetree/bindings/ata/tegra-sata.txt
>> new file mode 100644
>> index 0000000..946f207
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/ata/tegra-sata.txt
>> @@ -0,0 +1,30 @@
>> +Tegra124 SoC SATA AHCI controller
>> +
>> +Required properties :
>> +- compatible : "nvidia,tegra124-ahci".
>> +- reg : Should contain 2 entries:
>> +  - AHCI register set (SATA BAR5)
>> +  - SATA register set
>> +- interrupts : Defines the interrupt used by SATA
>> +- clocks : Must contain an entry for each entry in clock-names.
>> +  See ../clocks/clock-bindings.txt for details.
>> +- clock-names : Must include the following entries:
>> +  - sata
>> +  - sata-oob
>> +  - cml1
>> +  - pll_e
>
> It would be more readable if it like this...
>
> Required properties :
> - compatible	: "nvidia,tegra124-ahci".
> - reg		: Should contain 2 entries:
> 		  - AHCI register set (SATA BAR5)
> 		  - SATA register set
> - interrupts	: Defines the interrupt used by SATA
> - clocks	: Must contain an entry for each entry in clock-names.
> 		  See ../clocks/clock-bindings.txt for details.
> ....
>
>> +- resets : Must contain an entry for each entry in reset-names.
>> +  See ../reset/reset.txt for details.
>> +- reset-names : Must include the following entries:
>> +  - sata
>> +  - sata-oob
>> +  - sata-cold
>> +- phys : Must contain an entry for each entry in phy-names.
>> +  See ../phy/phy-bindings.txt for details.
>> +- phy-names : Must include the following entries:
>> +  - sata-phy : XUSB PADCTL SATA PHY
>> +- hvdd-supply : Defines the SATA HVDD regulator
>> +- vddio-supply : Defines the SATA VDDIO regulator
>> +- avdd-supply : Defines the SATA AVDD regulator
>> +- target-5v-supply : Defines the SATA 5V power regulator
>> +- target-12v-supply : Defines the SATA 12V power regulator
>
> Same....
>
>

Re: [PATCH v4 1/8] of: Add NVIDIA Tegra SATA controller binding

[Hans de Goede]

Hi,

On 07/16/2014 01:40 PM, Mikko Perttunen wrote:
> This patch adds device tree binding documentation for the SATA
> controller found on NVIDIA Tegra SoCs.
> 
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
> v4: clarify mandatory clock order

Thanks this and the new v4 of "ata: Add support for the Tegra124 SATA controller"
both look good to me. So these 2 + v3 for the rest of the series are:

Acked-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> 
>  .../devicetree/bindings/ata/tegra-sata.txt         | 31 ++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ata/tegra-sata.txt
> 
> diff --git a/Documentation/devicetree/bindings/ata/tegra-sata.txt b/Documentation/devicetree/bindings/ata/tegra-sata.txt
> new file mode 100644
> index 0000000..de2f775
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ata/tegra-sata.txt
> @@ -0,0 +1,31 @@
> +Tegra124 SoC SATA AHCI controller
> +
> +Required properties :
> +- compatible : "nvidia,tegra124-ahci".
> +- reg : Should contain 2 entries:
> +  - AHCI register set (SATA BAR5)
> +  - SATA register set
> +- interrupts : Defines the interrupt used by SATA
> +- clocks : Must contain an entry for each entry in clock-names.
> +  See ../clocks/clock-bindings.txt for details.
> +- clock-names : Must include the following entries in the same
> +  order as listed here:
> +  - sata
> +  - sata-oob
> +  - cml1
> +  - pll_e
> +- resets : Must contain an entry for each entry in reset-names.
> +  See ../reset/reset.txt for details.
> +- reset-names : Must include the following entries:
> +  - sata
> +  - sata-oob
> +  - sata-cold
> +- phys : Must contain an entry for each entry in phy-names.
> +  See ../phy/phy-bindings.txt for details.
> +- phy-names : Must include the following entries:
> +  - sata-phy : XUSB PADCTL SATA PHY
> +- hvdd-supply : Defines the SATA HVDD regulator
> +- vddio-supply : Defines the SATA VDDIO regulator
> +- avdd-supply : Defines the SATA AVDD regulator
> +- target-5v-supply : Defines the SATA 5V power regulator
> +- target-12v-supply : Defines the SATA 12V power regulator
> 

Re: [PATCH v4 1/8] of: Add NVIDIA Tegra SATA controller binding

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 671 bytes --]

On Wed, Jul 16, 2014 at 01:49:57PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 07/16/2014 01:40 PM, Mikko Perttunen wrote:
> > This patch adds device tree binding documentation for the SATA
> > controller found on NVIDIA Tegra SoCs.
> > 
> > Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> > ---
> > v4: clarify mandatory clock order
> 
> Thanks this and the new v4 of "ata: Add support for the Tegra124 SATA controller"
> both look good to me. So these 2 + v3 for the rest of the series are:
> 
> Acked-by: Hans de Goede <hdegoede@redhat.com>

Like I said in my reply to PATCH v3 7/8, I think this mandatory clock
order is a mistake.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 1/8] of: Add NVIDIA Tegra SATA controller binding

[Hans de Goede]

Hi,

On 07/16/2014 03:13 PM, Thierry Reding wrote:
> On Wed, Jul 16, 2014 at 01:49:57PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 07/16/2014 01:40 PM, Mikko Perttunen wrote:
>>> This patch adds device tree binding documentation for the SATA
>>> controller found on NVIDIA Tegra SoCs.
>>>
>>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>>> ---
>>> v4: clarify mandatory clock order
>>
>> Thanks this and the new v4 of "ata: Add support for the Tegra124 SATA controller"
>> both look good to me. So these 2 + v3 for the rest of the series are:
>>
>> Acked-by: Hans de Goede <hdegoede@redhat.com>
> 
> Like I said in my reply to PATCH v3 7/8, I think this mandatory clock
> order is a mistake.

We've plenty of other dt bindings where things need to be specified in
a certain order, e.g. registers. So I don't really see what the problem
is here.

Regards,

Hans

Re: [PATCH v4 1/8] of: Add NVIDIA Tegra SATA controller binding

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1311 bytes --]

On Wed, Jul 16, 2014 at 04:47:38PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 07/16/2014 03:13 PM, Thierry Reding wrote:
> > On Wed, Jul 16, 2014 at 01:49:57PM +0200, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 07/16/2014 01:40 PM, Mikko Perttunen wrote:
> >>> This patch adds device tree binding documentation for the SATA
> >>> controller found on NVIDIA Tegra SoCs.
> >>>
> >>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> >>> ---
> >>> v4: clarify mandatory clock order
> >>
> >> Thanks this and the new v4 of "ata: Add support for the Tegra124 SATA controller"
> >> both look good to me. So these 2 + v3 for the rest of the series are:
> >>
> >> Acked-by: Hans de Goede <hdegoede@redhat.com>
> > 
> > Like I said in my reply to PATCH v3 7/8, I think this mandatory clock
> > order is a mistake.
> 
> We've plenty of other dt bindings where things need to be specified in
> a certain order, e.g. registers. So I don't really see what the problem
> is here.

Like I said, the clock-names exists so that drivers can request a clock
by name. Therefore the order in which they are listed doesn't matter.
The only thing that matters is that the entries in clocks and
clock-names match up.

With the libahci_platform code we completely annul that convention.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v4 1/8] of: Add NVIDIA Tegra SATA controller binding

[Hans de Goede]

Hi,

On 07/16/2014 09:51 PM, Thierry Reding wrote:
> On Wed, Jul 16, 2014 at 04:47:38PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 07/16/2014 03:13 PM, Thierry Reding wrote:
>>> On Wed, Jul 16, 2014 at 01:49:57PM +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 07/16/2014 01:40 PM, Mikko Perttunen wrote:
>>>>> This patch adds device tree binding documentation for the SATA
>>>>> controller found on NVIDIA Tegra SoCs.
>>>>>
>>>>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>>>>> ---
>>>>> v4: clarify mandatory clock order
>>>>
>>>> Thanks this and the new v4 of "ata: Add support for the Tegra124 SATA controller"
>>>> both look good to me. So these 2 + v3 for the rest of the series are:
>>>>
>>>> Acked-by: Hans de Goede <hdegoede@redhat.com>
>>>
>>> Like I said in my reply to PATCH v3 7/8, I think this mandatory clock
>>> order is a mistake.
>>
>> We've plenty of other dt bindings where things need to be specified in
>> a certain order, e.g. registers. So I don't really see what the problem
>> is here.
> 
> Like I said, the clock-names exists so that drivers can request a clock
> by name. Therefore the order in which they are listed doesn't matter.
> The only thing that matters is that the entries in clocks and
> clock-names match up.

Ok so I've been think about this, and about the unbalance I've noticed
between tegra_ahci_power_on which does everything DIY and tegra_ahci_power_off
which uses ahci_platform_disable_resources() in v3 and later.

Really only the "sata" clock needs special handling, so I think the following
solution is best:

1) Drop the clock ordering requirement and the clk enum

2) Make ahci_tegra.c do a devm_clk_get(dev, "sata"), so that it gets its
own handle to the sata-clk (store this in tegra_ahci_priv). no need to
get all the other clks which need no special handling

3) Start using ahci_platform_enable_resources() in tegra_ahci_power_on,
so it would look something like this:

static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
{
	struct tegra_ahci_priv *tegra = hpriv->plat_data;
	int ret;

	ret = regulator_bulk_enable(ARRAY_SIZE(tegra->supplies),
				    tegra->supplies);
	if (ret)
		return ret;

	ret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_SATA,
						tegra->sata_clk,
						tegra->sata_rst);
	if (ret)
		goto disable_regulators;

	reset_control_deassert(tegra->sata_cold_rst);
	reset_control_deassert(tegra->sata_oob_rst);

	ret = ahci_platform_enable_resources(hpriv);
	if (ret)
		goto powergate_sequence_power_off;

	return 0;
...

This will make tegra_ahci_power_on and tegra_ahci_power_off symmetrical
which is something I always like to see in functions like this.

I realize that this changes the reset-deassert vs clock enabling ordering,
if this is an issue please add reset support to libahci-platform.c I believe
that is something which we will need to do soonish anyways (reset controllers
are popping up everywhere in newer SoCs).

This will nicely reduce the amount of code and also greatly simplify the error
return path of tegra_ahci_power_on.

This means that the sata_clk will get enabled twice, but that is harmless
as long as we disable it twice too. This means that we need to add an
extra disable to tegra_ahci_power_off because tegra_powergate_power_off
seems to not do this (unlike power-on, which is rather unsymmetrical
it would be nice to fix this).

Regards,

Hans

Re: [PATCH v4 1/8] of: Add NVIDIA Tegra SATA controller binding

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6293 bytes --]

On Thu, Jul 17, 2014 at 08:51:15AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 07/16/2014 09:51 PM, Thierry Reding wrote:
> > On Wed, Jul 16, 2014 at 04:47:38PM +0200, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 07/16/2014 03:13 PM, Thierry Reding wrote:
> >>> On Wed, Jul 16, 2014 at 01:49:57PM +0200, Hans de Goede wrote:
> >>>> Hi,
> >>>>
> >>>> On 07/16/2014 01:40 PM, Mikko Perttunen wrote:
> >>>>> This patch adds device tree binding documentation for the SATA
> >>>>> controller found on NVIDIA Tegra SoCs.
> >>>>>
> >>>>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> >>>>> ---
> >>>>> v4: clarify mandatory clock order
> >>>>
> >>>> Thanks this and the new v4 of "ata: Add support for the Tegra124 SATA controller"
> >>>> both look good to me. So these 2 + v3 for the rest of the series are:
> >>>>
> >>>> Acked-by: Hans de Goede <hdegoede@redhat.com>
> >>>
> >>> Like I said in my reply to PATCH v3 7/8, I think this mandatory clock
> >>> order is a mistake.
> >>
> >> We've plenty of other dt bindings where things need to be specified in
> >> a certain order, e.g. registers. So I don't really see what the problem
> >> is here.
> > 
> > Like I said, the clock-names exists so that drivers can request a clock
> > by name. Therefore the order in which they are listed doesn't matter.
> > The only thing that matters is that the entries in clocks and
> > clock-names match up.
> 
> Ok so I've been think about this, and about the unbalance I've noticed
> between tegra_ahci_power_on which does everything DIY and tegra_ahci_power_off
> which uses ahci_platform_disable_resources() in v3 and later.
> 
> Really only the "sata" clock needs special handling, so I think the following
> solution is best:
> 
> 1) Drop the clock ordering requirement and the clk enum
> 
> 2) Make ahci_tegra.c do a devm_clk_get(dev, "sata"), so that it gets its
> own handle to the sata-clk (store this in tegra_ahci_priv). no need to
> get all the other clks which need no special handling
> 
> 3) Start using ahci_platform_enable_resources() in tegra_ahci_power_on,
> so it would look something like this:
> 
> static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
> {
> 	struct tegra_ahci_priv *tegra = hpriv->plat_data;
> 	int ret;
> 
> 	ret = regulator_bulk_enable(ARRAY_SIZE(tegra->supplies),
> 				    tegra->supplies);
> 	if (ret)
> 		return ret;
> 
> 	ret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_SATA,
> 						tegra->sata_clk,
> 						tegra->sata_rst);
> 	if (ret)
> 		goto disable_regulators;
> 
> 	reset_control_deassert(tegra->sata_cold_rst);
> 	reset_control_deassert(tegra->sata_oob_rst);
> 
> 	ret = ahci_platform_enable_resources(hpriv);
> 	if (ret)
> 		goto powergate_sequence_power_off;
> 
> 	return 0;
> ...
> 
> This will make tegra_ahci_power_on and tegra_ahci_power_off symmetrical
> which is something I always like to see in functions like this.
> 
> I realize that this changes the reset-deassert vs clock enabling ordering,
> if this is an issue please add reset support to libahci-platform.c I believe
> that is something which we will need to do soonish anyways (reset controllers
> are popping up everywhere in newer SoCs).

I think we could safely move the reset deassert after the call to
ahci_platform_enable_resources().

> This will nicely reduce the amount of code and also greatly simplify the error
> return path of tegra_ahci_power_on.

Agreed, htat sounds like it could work.

> This means that the sata_clk will get enabled twice, but that is harmless
> as long as we disable it twice too. This means that we need to add an
> extra disable to tegra_ahci_power_off because tegra_powergate_power_off
> seems to not do this (unlike power-on, which is rather unsymmetrical
> it would be nice to fix this).

We've never had a need for it because the exact power down sequence
isn't nearly as important. But I guess we could add a new function
tegra_powergate_sequence_power_down() that takes care of disabling the
clock and asserting the reset.

One other thing that I've been thinking about is whether it would make
sense to make the ahci_platform library use a list of clock names that
it should request. This would better mirror the clock bindings
convention and allow drivers (such as the Tegra one) to take ownership
of clocks that need special handling while at the same time leaving it
to the helpers to do the bulk of the work.

One way I can think of to handle this would be by adding a struct
ahci_platform_resources * parameter to ahci_platform_get_resources(),
sowewhat like this:

	struct ahci_platform_resources {
		const char *const *clocks;
		unsigned int num_clocks;

		const char *const *resets;
		unsigned int num_resets;
	};

	struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
							   const struct ahci_platform_resources *res)
	{
		...

		for (i = 0; i < res->num_clocks; i++) {
			clk = clk_get(&pdev->dev, res->clocks[i]);
			...
		}

		...

		for (i = 0; i < res->num_resets; i++) {
			rst = reset_control_get(&pdev->dev, res->resets[i]);
			...
		}

		...
	}

And I guess the same could be done for regulators (and even phys). The
Tegra driver for instance could then do this:

	static const char *const tegra_ahci_clocks[] = {
		"sata-oob", "cml1", pll_e",
	};

	static const char *const tegra_ahci_resets[] = {
		"sata-oob", "sata-cold",
	};

	static const struct ahci_platform_resources tegra_ahci_resources = {
		.num_clocks = ARRAY_SIZE(tegra_ahci_clocks),
		.clocks = tegra_ahci_clocks,
		.num_resets = ARRAY_SIZE(tegra_ahci_resets),
		.resets = tegra_ahci_resets,
	};

	...

	struct tegra_ahci {
		struct ahci_host_priv *host;
		struct reset_control *rst;
		struct clk *clk;
		...
	};

	...

	static int tegra_ahci_probe(struct platform_device *pdev)
	{
		struct tegra_ahci *ahci;

		...

		ahci = devm_kzalloc(&pdev->dev, sizeof(*ahci), GFP_KERNEL);
		if (!ahci)
			return -ENOMEM;

		ahci->host = ahci_platform_get_resources(pdev, &tegra_ahci_resources);
		if (IS_ERR(ahci->host)) {
			...
		}

		...

		platform_set_drvdata(pdev, ahci);
	}

Does that sound reasonable?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 1/8] of: Add NVIDIA Tegra SATA controller binding

[Mikko Perttunen]

On 17/07/14 10:39, Thierry Reding wrote:
> ...
> One other thing that I've been thinking about is whether it would make
> sense to make the ahci_platform library use a list of clock names that
> it should request. This would better mirror the clock bindings
> convention and allow drivers (such as the Tegra one) to take ownership
> of clocks that need special handling while at the same time leaving it
> to the helpers to do the bulk of the work.
>
> One way I can think of to handle this would be by adding a struct
> ahci_platform_resources * parameter to ahci_platform_get_resources(),
> sowewhat like this:
>
> 	struct ahci_platform_resources {
> 		const char *const *clocks;
> 		unsigned int num_clocks;
>
> 		const char *const *resets;
> 		unsigned int num_resets;
> 	};
>
> 	struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
> 							   const struct ahci_platform_resources *res)
> 	{
> 		...
>
> 		for (i = 0; i < res->num_clocks; i++) {
> 			clk = clk_get(&pdev->dev, res->clocks[i]);
> 			...
> 		}
>
> 		...
>
> 		for (i = 0; i < res->num_resets; i++) {
> 			rst = reset_control_get(&pdev->dev, res->resets[i]);
> 			...
> 		}
>
> 		...
> 	}
>

I think something like this would be required to support reset_controls 
anyway, as you can only get reset controls by name. This is what I 
alluded to (in the cover letter) when saying that adding reset control 
support would require an API break.

Also: is there a reason to not use the devm_* variants? I note that the 
helper code has not been able to prevent any of the ahci_platform 
drivers from messing up by not calling ahci_platform_put_resources.

- Mikko

Re: [PATCH v4 1/8] of: Add NVIDIA Tegra SATA controller binding

[Hans de Goede]

Hi,

On 07/17/2014 09:39 AM, Thierry Reding wrote:
> On Thu, Jul 17, 2014 at 08:51:15AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 07/16/2014 09:51 PM, Thierry Reding wrote:
>>> On Wed, Jul 16, 2014 at 04:47:38PM +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 07/16/2014 03:13 PM, Thierry Reding wrote:
>>>>> On Wed, Jul 16, 2014 at 01:49:57PM +0200, Hans de Goede wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 07/16/2014 01:40 PM, Mikko Perttunen wrote:
>>>>>>> This patch adds device tree binding documentation for the SATA
>>>>>>> controller found on NVIDIA Tegra SoCs.
>>>>>>>
>>>>>>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>>>>>>> ---
>>>>>>> v4: clarify mandatory clock order
>>>>>>
>>>>>> Thanks this and the new v4 of "ata: Add support for the Tegra124 SATA controller"
>>>>>> both look good to me. So these 2 + v3 for the rest of the series are:
>>>>>>
>>>>>> Acked-by: Hans de Goede <hdegoede@redhat.com>
>>>>>
>>>>> Like I said in my reply to PATCH v3 7/8, I think this mandatory clock
>>>>> order is a mistake.
>>>>
>>>> We've plenty of other dt bindings where things need to be specified in
>>>> a certain order, e.g. registers. So I don't really see what the problem
>>>> is here.
>>>
>>> Like I said, the clock-names exists so that drivers can request a clock
>>> by name. Therefore the order in which they are listed doesn't matter.
>>> The only thing that matters is that the entries in clocks and
>>> clock-names match up.
>>
>> Ok so I've been think about this, and about the unbalance I've noticed
>> between tegra_ahci_power_on which does everything DIY and tegra_ahci_power_off
>> which uses ahci_platform_disable_resources() in v3 and later.
>>
>> Really only the "sata" clock needs special handling, so I think the following
>> solution is best:
>>
>> 1) Drop the clock ordering requirement and the clk enum
>>
>> 2) Make ahci_tegra.c do a devm_clk_get(dev, "sata"), so that it gets its
>> own handle to the sata-clk (store this in tegra_ahci_priv). no need to
>> get all the other clks which need no special handling
>>
>> 3) Start using ahci_platform_enable_resources() in tegra_ahci_power_on,
>> so it would look something like this:
>>
>> static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
>> {
>> 	struct tegra_ahci_priv *tegra = hpriv->plat_data;
>> 	int ret;
>>
>> 	ret = regulator_bulk_enable(ARRAY_SIZE(tegra->supplies),
>> 				    tegra->supplies);
>> 	if (ret)
>> 		return ret;
>>
>> 	ret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_SATA,
>> 						tegra->sata_clk,
>> 						tegra->sata_rst);
>> 	if (ret)
>> 		goto disable_regulators;
>>
>> 	reset_control_deassert(tegra->sata_cold_rst);
>> 	reset_control_deassert(tegra->sata_oob_rst);
>>
>> 	ret = ahci_platform_enable_resources(hpriv);
>> 	if (ret)
>> 		goto powergate_sequence_power_off;
>>
>> 	return 0;
>> ...
>>
>> This will make tegra_ahci_power_on and tegra_ahci_power_off symmetrical
>> which is something I always like to see in functions like this.
>>
>> I realize that this changes the reset-deassert vs clock enabling ordering,
>> if this is an issue please add reset support to libahci-platform.c I believe
>> that is something which we will need to do soonish anyways (reset controllers
>> are popping up everywhere in newer SoCs).
> 
> I think we could safely move the reset deassert after the call to
> ahci_platform_enable_resources().

Will the resets not interfere with the phy_init / power_on which is done from
ahci_platform_enable_resources().

>> This will nicely reduce the amount of code and also greatly simplify the error
>> return path of tegra_ahci_power_on.
> 
> Agreed, htat sounds like it could work.
> 
>> This means that the sata_clk will get enabled twice, but that is harmless
>> as long as we disable it twice too. This means that we need to add an
>> extra disable to tegra_ahci_power_off because tegra_powergate_power_off
>> seems to not do this (unlike power-on, which is rather unsymmetrical
>> it would be nice to fix this).
> 
> We've never had a need for it because the exact power down sequence
> isn't nearly as important. But I guess we could add a new function
> tegra_powergate_sequence_power_down() that takes care of disabling the
> clock and asserting the reset.

Ok, no need to add it for my sake, I was just wondering about the non
symmetry of the API.

> One other thing that I've been thinking about is whether it would make
> sense to make the ahci_platform library use a list of clock names that
> it should request. This would better mirror the clock bindings
> convention and allow drivers (such as the Tegra one) to take ownership
> of clocks that need special handling while at the same time leaving it
> to the helpers to do the bulk of the work.
> 
> One way I can think of to handle this would be by adding a struct
> ahci_platform_resources * parameter to ahci_platform_get_resources(),
> sowewhat like this:
> 
> 	struct ahci_platform_resources {
> 		const char *const *clocks;
> 		unsigned int num_clocks;
> 
> 		const char *const *resets;
> 		unsigned int num_resets;
> 	};
> 
> 	struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
> 							   const struct ahci_platform_resources *res)
> 	{
> 		...
> 
> 		for (i = 0; i < res->num_clocks; i++) {
> 			clk = clk_get(&pdev->dev, res->clocks[i]);
> 			...
> 		}
> 
> 		...
> 
> 		for (i = 0; i < res->num_resets; i++) {
> 			rst = reset_control_get(&pdev->dev, res->resets[i]);
> 			...
> 		}
> 
> 		...
> 	}

Interesting, I think this is a quite good idea. I do see some pitfalls though,
to answers Mikko's question from his followup :

On 07/17/2014 09:56 AM, Mikko Perttunen wrote:
> Also: is there a reason to not use the devm_* variants? I note that the helper code has not been able to prevent any of the ahci_platform drivers from messing up by not calling ahci_platform_put_resources.

The libahci_platform.c code / ahci_platform.c code is also used for
devices going way back who may not yet be using the new clk framework,
so where we need to use clk_get(dev, NULL); quoting from libahci_platform.c :

        for (i = 0; i < AHCI_MAX_CLKS; i++) {
                /*
                 * For now we must use clk_get(dev, NULL) for the first clock,
                 * because some platforms (da850, spear13xx) are not yet
                 * converted to use devicetree for clocks.  For new platforms
                 * this is equivalent to of_clk_get(dev->of_node, 0).
                 */
                if (i == 0)
                        clk = clk_get(dev, NULL);
                else
                        clk = of_clk_get(dev->of_node, i);

                if (IS_ERR(clk)) {
                        rc = PTR_ERR(clk);
                        if (rc == -EPROBE_DEFER)
                                goto err_out;
                        break;
                }
                hpriv->clks[i] = clk;
        }

And there is no devm variant of that, nor is there one to get clocks by index.
Note that we also need ahci_platform_put_resources for runtime pm support, so
that one is going to stay around anyways and thus there is not that much value
in fixing this.

So although I like Thierry's idea, if we go this way (which sounds good), we
should add support for taking a NULL ahci_platform_resources argument and in
that case behave as before, esp. because of the platforms needing the old
style clock handling. An advantage of doing this, is that we can simply patch
all existing users to pass NULL.

I guess we could have a default ahci_platform_resources struct which gets
used when the argument is NULL, then we only need to special case the clocks
stuff when the argument is NULL.

> And I guess the same could be done for regulators (and even phys). The
> Tegra driver for instance could then do this:

Ack.


> 	static const char *const tegra_ahci_clocks[] = {
> 		"sata-oob", "cml1", pll_e",
> 	};

Note you could also put the "sata" clock here, since then you will
know its index, and can use hpriv->clks to access it. Not putting
it here has the advantage of not doing the double enable / disable
(and the disadvantage of needing to the clk_get yourself).

> 	static const char *const tegra_ahci_resets[] = {
> 		"sata-oob", "sata-cold",
> 	};
> 
> 	static const struct ahci_platform_resources tegra_ahci_resources = {
> 		.num_clocks = ARRAY_SIZE(tegra_ahci_clocks),
> 		.clocks = tegra_ahci_clocks,
> 		.num_resets = ARRAY_SIZE(tegra_ahci_resets),
> 		.resets = tegra_ahci_resets,
> 	};
> 
> 	...
> 
> 	struct tegra_ahci {
> 		struct ahci_host_priv *host;
> 		struct reset_control *rst;
> 		struct clk *clk;
> 		...
> 	};
> 
> 	...
> 
> 	static int tegra_ahci_probe(struct platform_device *pdev)
> 	{
> 		struct tegra_ahci *ahci;
> 
> 		...
> 
> 		ahci = devm_kzalloc(&pdev->dev, sizeof(*ahci), GFP_KERNEL);
> 		if (!ahci)
> 			return -ENOMEM;
> 
> 		ahci->host = ahci_platform_get_resources(pdev, &tegra_ahci_resources);
> 		if (IS_ERR(ahci->host)) {
> 			...
> 		}
> 
> 		...
> 
> 		platform_set_drvdata(pdev, ahci);
> 	}
> 
> Does that sound reasonable?

Yep, sounds good to me (with the caveat of the back-compat stuff).

Regards,

Hans

Re: [PATCH v4 1/8] of: Add NVIDIA Tegra SATA controller binding

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6033 bytes --]

On Thu, Jul 17, 2014 at 12:23:47PM +0200, Hans de Goede wrote:
> On 07/17/2014 09:39 AM, Thierry Reding wrote:
> > On Thu, Jul 17, 2014 at 08:51:15AM +0200, Hans de Goede wrote:
[...]
> > > I realize that this changes the reset-deassert vs clock enabling ordering,
> > > if this is an issue please add reset support to libahci-platform.c I believe
> > > that is something which we will need to do soonish anyways (reset controllers
> > > are popping up everywhere in newer SoCs).
> > 
> > I think we could safely move the reset deassert after the call to
> > ahci_platform_enable_resources().
> 
> Will the resets not interfere with the phy_init / power_on which is done from
> ahci_platform_enable_resources().

The PHY is a completely different hardware block and I think the only
requisite is that the regulator be turned on. Resets from SATA don't
influence it.

> >> This means that the sata_clk will get enabled twice, but that is harmless
> >> as long as we disable it twice too. This means that we need to add an
> >> extra disable to tegra_ahci_power_off because tegra_powergate_power_off
> >> seems to not do this (unlike power-on, which is rather unsymmetrical
> >> it would be nice to fix this).
> > 
> > We've never had a need for it because the exact power down sequence
> > isn't nearly as important. But I guess we could add a new function
> > tegra_powergate_sequence_power_down() that takes care of disabling the
> > clock and asserting the reset.
> 
> Ok, no need to add it for my sake, I was just wondering about the non
> symmetry of the API.

There are discussions on how to make powergates work with PM domains.
They aren't an ideal fit, but if we manage to make it work the powergate
API will likely go away anyway.

> > One other thing that I've been thinking about is whether it would make
> > sense to make the ahci_platform library use a list of clock names that
> > it should request. This would better mirror the clock bindings
> > convention and allow drivers (such as the Tegra one) to take ownership
> > of clocks that need special handling while at the same time leaving it
> > to the helpers to do the bulk of the work.
> > 
> > One way I can think of to handle this would be by adding a struct
> > ahci_platform_resources * parameter to ahci_platform_get_resources(),
> > sowewhat like this:
> > 
> > 	struct ahci_platform_resources {
> > 		const char *const *clocks;
> > 		unsigned int num_clocks;
> > 
> > 		const char *const *resets;
> > 		unsigned int num_resets;
> > 	};
> > 
> > 	struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
> > 							   const struct ahci_platform_resources *res)
> > 	{
> > 		...
> > 
> > 		for (i = 0; i < res->num_clocks; i++) {
> > 			clk = clk_get(&pdev->dev, res->clocks[i]);
> > 			...
> > 		}
> > 
> > 		...
> > 
> > 		for (i = 0; i < res->num_resets; i++) {
> > 			rst = reset_control_get(&pdev->dev, res->resets[i]);
> > 			...
> > 		}
> > 
> > 		...
> > 	}
> 
> Interesting, I think this is a quite good idea. I do see some pitfalls though,
> to answers Mikko's question from his followup :
> 
> On 07/17/2014 09:56 AM, Mikko Perttunen wrote:
> > Also: is there a reason to not use the devm_* variants? I note that the helper code has not been able to prevent any of the ahci_platform drivers from messing up by not calling ahci_platform_put_resources.
> 
> The libahci_platform.c code / ahci_platform.c code is also used for
> devices going way back who may not yet be using the new clk framework,
> so where we need to use clk_get(dev, NULL); quoting from libahci_platform.c :
> 
>         for (i = 0; i < AHCI_MAX_CLKS; i++) {
>                 /*
>                  * For now we must use clk_get(dev, NULL) for the first clock,
>                  * because some platforms (da850, spear13xx) are not yet
>                  * converted to use devicetree for clocks.  For new platforms
>                  * this is equivalent to of_clk_get(dev->of_node, 0).
>                  */
>                 if (i == 0)
>                         clk = clk_get(dev, NULL);
>                 else
>                         clk = of_clk_get(dev->of_node, i);
> 
>                 if (IS_ERR(clk)) {
>                         rc = PTR_ERR(clk);
>                         if (rc == -EPROBE_DEFER)
>                                 goto err_out;
>                         break;
>                 }
>                 hpriv->clks[i] = clk;
>         }
> 
> And there is no devm variant of that, nor is there one to get clocks by index.
> Note that we also need ahci_platform_put_resources for runtime pm support, so
> that one is going to stay around anyways and thus there is not that much value
> in fixing this.
> 
> So although I like Thierry's idea, if we go this way (which sounds good), we
> should add support for taking a NULL ahci_platform_resources argument and in
> that case behave as before, esp. because of the platforms needing the old
> style clock handling. An advantage of doing this, is that we can simply patch
> all existing users to pass NULL.

Isn't the "legacy" case really just this:

	static const char *const legacy_ahci_clocks[] = {
		NULL
	};

	static const struct ahci_platform_resources legacy_ahci_resources = {
		.num_clocks = ARRAY_SIZE(legacy_ahci_clocks),
		.clocks = legacy_ahci_clocks,
	};

?

> > 	static const char *const tegra_ahci_clocks[] = {
> > 		"sata-oob", "cml1", pll_e",
> > 	};
> 
> Note you could also put the "sata" clock here, since then you will
> know its index, and can use hpriv->clks to access it. Not putting
> it here has the advantage of not doing the double enable / disable
> (and the disadvantage of needing to the clk_get yourself).

Right, the intention here was that we wouldn't have it managed by the
ahci_platform library because it needs special handling anyway. Having
it in this table would therefore be redundant.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 1/8] of: Add NVIDIA Tegra SATA controller binding

[Mikko Perttunen]

On 17/07/14 13:23, Hans de Goede wrote:
> On 07/17/2014 09:56 AM, Mikko Perttunen wrote:
>> Also: is there a reason to not use the devm_* variants? I note that the helper code has not been able to prevent any of the ahci_platform drivers from messing up by not calling ahci_platform_put_resources.
>
> The libahci_platform.c code / ahci_platform.c code is also used for
> devices going way back who may not yet be using the new clk framework,
> so where we need to use clk_get(dev, NULL); quoting from libahci_platform.c :
>
>          for (i = 0; i < AHCI_MAX_CLKS; i++) {
>                  /*
>                   * For now we must use clk_get(dev, NULL) for the first clock,
>                   * because some platforms (da850, spear13xx) are not yet
>                   * converted to use devicetree for clocks.  For new platforms
>                   * this is equivalent to of_clk_get(dev->of_node, 0).
>                   */
>                  if (i == 0)
>                          clk = clk_get(dev, NULL);
>                  else
>                          clk = of_clk_get(dev->of_node, i);
>
>                  if (IS_ERR(clk)) {
>                          rc = PTR_ERR(clk);
>                          if (rc == -EPROBE_DEFER)
>                                  goto err_out;
>                          break;
>                  }
>                  hpriv->clks[i] = clk;
>          }
>
> And there is no devm variant of that, nor is there one to get clocks by index.
> Note that we also need ahci_platform_put_resources for runtime pm support, so
> that one is going to stay around anyways and thus there is not that much value
> in fixing this.
>

Ah, and looks like devres is used to call put_resources anyway. My mistake.

Re: [PATCH v4 1/8] of: Add NVIDIA Tegra SATA controller binding

[Hans de Goede]

Hi,

On 07/17/2014 12:52 PM, Thierry Reding wrote:
> On Thu, Jul 17, 2014 at 12:23:47PM +0200, Hans de Goede wrote:

<snip>

>> The libahci_platform.c code / ahci_platform.c code is also used for
>> devices going way back who may not yet be using the new clk framework,
>> so where we need to use clk_get(dev, NULL); quoting from libahci_platform.c :
>>
>>         for (i = 0; i < AHCI_MAX_CLKS; i++) {
>>                 /*
>>                  * For now we must use clk_get(dev, NULL) for the first clock,
>>                  * because some platforms (da850, spear13xx) are not yet
>>                  * converted to use devicetree for clocks.  For new platforms
>>                  * this is equivalent to of_clk_get(dev->of_node, 0).
>>                  */
>>                 if (i == 0)
>>                         clk = clk_get(dev, NULL);
>>                 else
>>                         clk = of_clk_get(dev->of_node, i);
>>
>>                 if (IS_ERR(clk)) {
>>                         rc = PTR_ERR(clk);
>>                         if (rc == -EPROBE_DEFER)
>>                                 goto err_out;
>>                         break;
>>                 }
>>                 hpriv->clks[i] = clk;
>>         }
>>
>> And there is no devm variant of that, nor is there one to get clocks by index.
>> Note that we also need ahci_platform_put_resources for runtime pm support, so
>> that one is going to stay around anyways and thus there is not that much value
>> in fixing this.
>>
>> So although I like Thierry's idea, if we go this way (which sounds good), we
>> should add support for taking a NULL ahci_platform_resources argument and in
>> that case behave as before, esp. because of the platforms needing the old
>> style clock handling. An advantage of doing this, is that we can simply patch
>> all existing users to pass NULL.
> 
> Isn't the "legacy" case really just this:
> 
> 	static const char *const legacy_ahci_clocks[] = {
> 		NULL
> 	};
> 
> 	static const struct ahci_platform_resources legacy_ahci_resources = {
> 		.num_clocks = ARRAY_SIZE(legacy_ahci_clocks),
> 		.clocks = legacy_ahci_clocks,
> 	};
> 
> ?

Ah yes that would work for the really legacy ones, as well as less legacy
(full dts) ones with only one clk, we need to check if there are current
users which use more then one clk (yes there are which is why MAX_CLKS
was 3) and fixup those to pass in a correct ahci_platform_resources struct
then.

The checking + fixing up will be a bit of extra work, but I think the end result
will be quite nice. so I'm all in favor of this.

>>> 	static const char *const tegra_ahci_clocks[] = {
>>> 		"sata-oob", "cml1", pll_e",
>>> 	};
>>
>> Note you could also put the "sata" clock here, since then you will
>> know its index, and can use hpriv->clks to access it. Not putting
>> it here has the advantage of not doing the double enable / disable
>> (and the disadvantage of needing to the clk_get yourself).
> 
> Right, the intention here was that we wouldn't have it managed by the
> ahci_platform library because it needs special handling anyway. Having
> it in this table would therefore be redundant.

Ack.

Regards,

Hans

Re: [PATCH v4 1/8] of: Add NVIDIA Tegra SATA controller binding

[Hans de Goede]

Hi,

On 07/17/2014 01:42 PM, Hans de Goede wrote:
> Hi,
> 
> On 07/17/2014 12:52 PM, Thierry Reding wrote:
>> On Thu, Jul 17, 2014 at 12:23:47PM +0200, Hans de Goede wrote:
> 
> <snip>
> 
>>> The libahci_platform.c code / ahci_platform.c code is also used for
>>> devices going way back who may not yet be using the new clk framework,
>>> so where we need to use clk_get(dev, NULL); quoting from libahci_platform.c :
>>>
>>>         for (i = 0; i < AHCI_MAX_CLKS; i++) {
>>>                 /*
>>>                  * For now we must use clk_get(dev, NULL) for the first clock,
>>>                  * because some platforms (da850, spear13xx) are not yet
>>>                  * converted to use devicetree for clocks.  For new platforms
>>>                  * this is equivalent to of_clk_get(dev->of_node, 0).
>>>                  */
>>>                 if (i == 0)
>>>                         clk = clk_get(dev, NULL);
>>>                 else
>>>                         clk = of_clk_get(dev->of_node, i);
>>>
>>>                 if (IS_ERR(clk)) {
>>>                         rc = PTR_ERR(clk);
>>>                         if (rc == -EPROBE_DEFER)
>>>                                 goto err_out;
>>>                         break;
>>>                 }
>>>                 hpriv->clks[i] = clk;
>>>         }
>>>
>>> And there is no devm variant of that, nor is there one to get clocks by index.
>>> Note that we also need ahci_platform_put_resources for runtime pm support, so
>>> that one is going to stay around anyways and thus there is not that much value
>>> in fixing this.
>>>
>>> So although I like Thierry's idea, if we go this way (which sounds good), we
>>> should add support for taking a NULL ahci_platform_resources argument and in
>>> that case behave as before, esp. because of the platforms needing the old
>>> style clock handling. An advantage of doing this, is that we can simply patch
>>> all existing users to pass NULL.
>>
>> Isn't the "legacy" case really just this:
>>
>> 	static const char *const legacy_ahci_clocks[] = {
>> 		NULL
>> 	};
>>
>> 	static const struct ahci_platform_resources legacy_ahci_resources = {
>> 		.num_clocks = ARRAY_SIZE(legacy_ahci_clocks),
>> 		.clocks = legacy_ahci_clocks,
>> 	};
>>
>> ?
> 
> Ah yes that would work for the really legacy ones, as well as less legacy
> (full dts) ones with only one clk, we need to check if there are current
> users which use more then one clk (yes there are which is why MAX_CLKS
> was 3) and fixup those to pass in a correct ahci_platform_resources struct
> then.
> 
> The checking + fixing up will be a bit of extra work, but I think the end result
> will be quite nice. so I'm all in favor of this.

Correction, this is not going to work I'm afraid, as not all current dts files
set clock-names. So we need a fallback to get clks by index for compatibility
with old dts files.

At least: arch/arm/boot/dts/sun4i-a10.dtsi and arch/arm/boot/dts/sun7i-a20.dtsi
are affected. Note in this case the dts file is typically not burned into
a ROM or some such, so IMHO we could get away with requiring a new dts file.

So it might be worthwhile to still do a full check if all affected SoCs and
see if we can move over to using clock-names for all platforms with
more then 1 ahci/sata clk.

FWIW I've also just checked imx6q.dts which is the one which has 3 clocks, and
that one does define clk names.

Regards,

Hans

[PATCH v5 1/8] of: Add NVIDIA Tegra SATA controller binding

[Mikko Perttunen]

This patch adds device tree binding documentation for the SATA
controller found on NVIDIA Tegra SoCs.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
v5: remove ordering requirement again

 .../devicetree/bindings/ata/tegra-sata.txt         | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ata/tegra-sata.txt

diff --git a/Documentation/devicetree/bindings/ata/tegra-sata.txt b/Documentation/devicetree/bindings/ata/tegra-sata.txt
new file mode 100644
index 0000000..946f207
--- /dev/null
+++ b/Documentation/devicetree/bindings/ata/tegra-sata.txt
@@ -0,0 +1,30 @@
+Tegra124 SoC SATA AHCI controller
+
+Required properties :
+- compatible : "nvidia,tegra124-ahci".
+- reg : Should contain 2 entries:
+  - AHCI register set (SATA BAR5)
+  - SATA register set
+- interrupts : Defines the interrupt used by SATA
+- clocks : Must contain an entry for each entry in clock-names.
+  See ../clocks/clock-bindings.txt for details.
+- clock-names : Must include the following entries:
+  - sata
+  - sata-oob
+  - cml1
+  - pll_e
+- resets : Must contain an entry for each entry in reset-names.
+  See ../reset/reset.txt for details.
+- reset-names : Must include the following entries:
+  - sata
+  - sata-oob
+  - sata-cold
+- phys : Must contain an entry for each entry in phy-names.
+  See ../phy/phy-bindings.txt for details.
+- phy-names : Must include the following entries:
+  - sata-phy : XUSB PADCTL SATA PHY
+- hvdd-supply : Defines the SATA HVDD regulator
+- vddio-supply : Defines the SATA VDDIO regulator
+- avdd-supply : Defines the SATA AVDD regulator
+- target-5v-supply : Defines the SATA 5V power regulator
+- target-12v-supply : Defines the SATA 12V power regulator
-- 
1.8.1.5

[PATCH v5 7/8] ata: Add support for the Tegra124 SATA controller

[Mikko Perttunen]

This adds support for the integrated AHCI-compliant Serial ATA
controller present on the NVIDIA Tegra124 system-on-chip.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
v5: let ahci_platform handle sata clock and also handle it ourselves.
  this allows use of ahci_platform while having a special sequence
  for the clock.

 drivers/ata/Kconfig      |   9 ++
 drivers/ata/Makefile     |   1 +
 drivers/ata/ahci_tegra.c | 377 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 387 insertions(+)
 create mode 100644 drivers/ata/ahci_tegra.c

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index b0d5b5a..e1b9278 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -142,6 +142,15 @@ config AHCI_SUNXI
 
 	  If unsure, say N.
 
+config AHCI_TEGRA
+	tristate "NVIDIA Tegra124 AHCI SATA support"
+	depends on ARCH_TEGRA
+	help
+	  This option enables support for the NVIDIA Tegra124 SoC's
+	  onboard AHCI SATA.
+
+	  If unsure, say N.
+
 config AHCI_XGENE
 	tristate "APM X-Gene 6.0Gbps AHCI SATA host controller support"
 	depends on PHY_XGENE
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 5a02aee..ae41107 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_AHCI_IMX)		+= ahci_imx.o libahci.o libahci_platform.o
 obj-$(CONFIG_AHCI_MVEBU)	+= ahci_mvebu.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
+obj-$(CONFIG_AHCI_TEGRA)	+= ahci_tegra.o libahci.o libahci_platform.o
 obj-$(CONFIG_AHCI_XGENE)	+= ahci_xgene.o libahci.o libahci_platform.o
 
 # SFF w/ custom DMA
diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c
new file mode 100644
index 0000000..d30bb21
--- /dev/null
+++ b/drivers/ata/ahci_tegra.c
@@ -0,0 +1,377 @@
+/*
+ * drivers/ata/ahci_tegra.c
+ *
+ * Copyright (c) 2014, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * Author:
+ *	Mikko Perttunen <mperttunen@nvidia.com>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/ahci_platform.h>
+#include <linux/reset.h>
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/tegra-powergate.h>
+#include <linux/regulator/consumer.h>
+#include "ahci.h"
+
+#define SATA_CONFIGURATION_0				0x180
+#define SATA_CONFIGURATION_EN_FPCI			BIT(0)
+
+#define SCFG_OFFSET					0x1000
+
+#define T_SATA0_CFG_1					0x04
+#define T_SATA0_CFG_1_IO_SPACE				BIT(0)
+#define T_SATA0_CFG_1_MEMORY_SPACE			BIT(1)
+#define T_SATA0_CFG_1_BUS_MASTER			BIT(2)
+#define T_SATA0_CFG_1_SERR				BIT(8)
+
+#define T_SATA0_CFG_9					0x24
+#define T_SATA0_CFG_9_BASE_ADDRESS_SHIFT		13
+
+#define SATA_FPCI_BAR5					0x94
+#define SATA_FPCI_BAR5_START_SHIFT			4
+
+#define SATA_INTR_MASK					0x188
+#define SATA_INTR_MASK_IP_INT_MASK			BIT(16)
+
+#define T_SATA0_AHCI_HBA_CAP_BKDR			0x300
+
+#define T_SATA0_BKDOOR_CC				0x4a4
+
+#define T_SATA0_CFG_SATA				0x54c
+#define T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN		BIT(12)
+
+#define T_SATA0_CFG_MISC				0x550
+
+#define T_SATA0_INDEX					0x680
+
+#define T_SATA0_CHX_PHY_CTRL1_GEN1			0x690
+#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK		0xff
+#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT		0
+#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK		(0xff << 8)
+#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT	8
+
+#define T_SATA0_CHX_PHY_CTRL1_GEN2			0x694
+#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_MASK		0xff
+#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_SHIFT		0
+#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_MASK		(0xff << 12)
+#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_SHIFT	12
+
+#define T_SATA0_CHX_PHY_CTRL2				0x69c
+#define T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1		0x23
+
+#define T_SATA0_CHX_PHY_CTRL11				0x6d0
+#define T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ		(0x2800 << 16)
+
+#define FUSE_SATA_CALIB					0x124
+#define FUSE_SATA_CALIB_MASK				0x3
+
+struct sata_pad_calibration {
+	u8 gen1_tx_amp;
+	u8 gen1_tx_peak;
+	u8 gen2_tx_amp;
+	u8 gen2_tx_peak;
+};
+
+static const struct sata_pad_calibration tegra124_pad_calibration[] = {
+	{0x18, 0x04, 0x18, 0x0a},
+	{0x0e, 0x04, 0x14, 0x0a},
+	{0x0e, 0x07, 0x1a, 0x0e},
+	{0x14, 0x0e, 0x1a, 0x0e},
+};
+
+struct tegra_ahci_priv {
+	struct platform_device	   *pdev;
+	void __iomem		   *sata_regs;
+	struct reset_control	   *sata_rst;
+	struct reset_control	   *sata_oob_rst;
+	struct reset_control	   *sata_cold_rst;
+	/* Needs special handling, cannot use ahci_platform */
+	struct clk		   *sata_clk;
+	struct regulator_bulk_data supplies[5];
+};
+
+static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
+{
+	struct tegra_ahci_priv *tegra = hpriv->plat_data;
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(tegra->supplies),
+				    tegra->supplies);
+	if (ret)
+		return ret;
+
+	ret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_SATA,
+						tegra->sata_clk,
+						tegra->sata_rst);
+	if (ret)
+		goto disable_regulators;
+
+	reset_control_assert(tegra->sata_oob_rst);
+	reset_control_assert(tegra->sata_cold_rst);
+
+	ret = ahci_platform_enable_resources(hpriv);
+	if (ret)
+		goto disable_power;
+
+	reset_control_deassert(tegra->sata_cold_rst);
+	reset_control_deassert(tegra->sata_oob_rst);
+
+	return 0;
+
+disable_power:
+	clk_disable_unprepare(tegra->sata_clk);
+
+	tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
+
+disable_regulators:
+	regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra->supplies);
+
+	return ret;
+}
+
+static void tegra_ahci_power_off(struct ahci_host_priv *hpriv)
+{
+	struct tegra_ahci_priv *tegra = hpriv->plat_data;
+
+	ahci_platform_disable_resources(hpriv);
+
+	reset_control_assert(tegra->sata_rst);
+	reset_control_assert(tegra->sata_oob_rst);
+	reset_control_assert(tegra->sata_cold_rst);
+
+	clk_disable_unprepare(tegra->sata_clk);
+	tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
+
+	regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra->supplies);
+}
+
+static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv)
+{
+	struct tegra_ahci_priv *tegra = hpriv->plat_data;
+	int ret;
+	unsigned int val;
+	struct sata_pad_calibration calib;
+
+	ret = tegra_ahci_power_on(hpriv);
+	if (ret) {
+		dev_err(&tegra->pdev->dev,
+			"failed to power on AHCI controller: %d\n", ret);
+		return ret;
+	}
+
+	val = readl(tegra->sata_regs + SATA_CONFIGURATION_0);
+	val |= SATA_CONFIGURATION_EN_FPCI;
+	writel(val, tegra->sata_regs + SATA_CONFIGURATION_0);
+
+	/* Pad calibration */
+
+	/* FIXME Always use calibration 0. Change this to read the calibration
+	 * fuse once the fuse driver has landed. */
+	val = 0;
+
+	calib = tegra124_pad_calibration[val & FUSE_SATA_CALIB_MASK];
+
+	writel(BIT(0), tegra->sata_regs + SCFG_OFFSET + T_SATA0_INDEX);
+
+	val = readl(tegra->sata_regs +
+		SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN1);
+	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK;
+	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK;
+	val |= calib.gen1_tx_amp <<
+			T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT;
+	val |= calib.gen1_tx_peak <<
+			T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT;
+	writel(val, tegra->sata_regs + SCFG_OFFSET +
+		T_SATA0_CHX_PHY_CTRL1_GEN1);
+
+	val = readl(tegra->sata_regs +
+			SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN2);
+	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_MASK;
+	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_MASK;
+	val |= calib.gen2_tx_amp <<
+			T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT;
+	val |= calib.gen2_tx_peak <<
+			T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT;
+	writel(val, tegra->sata_regs + SCFG_OFFSET +
+		T_SATA0_CHX_PHY_CTRL1_GEN2);
+
+	writel(T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ,
+		tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL11);
+	writel(T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1,
+		tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL2);
+
+	writel(0, tegra->sata_regs + SCFG_OFFSET + T_SATA0_INDEX);
+
+	/* Program controller device ID */
+
+	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
+	val |= T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN;
+	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
+
+	writel(0x01060100, tegra->sata_regs + SCFG_OFFSET + T_SATA0_BKDOOR_CC);
+
+	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
+	val &= ~T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN;
+	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
+
+	/* Enable IO & memory access, bus master mode */
+
+	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_1);
+	val |= T_SATA0_CFG_1_IO_SPACE | T_SATA0_CFG_1_MEMORY_SPACE |
+		T_SATA0_CFG_1_BUS_MASTER | T_SATA0_CFG_1_SERR;
+	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_1);
+
+	/* Program SATA MMIO */
+
+	writel(0x10000 << SATA_FPCI_BAR5_START_SHIFT,
+	       tegra->sata_regs + SATA_FPCI_BAR5);
+
+	writel(0x08000 << T_SATA0_CFG_9_BASE_ADDRESS_SHIFT,
+	       tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_9);
+
+	/* Unmask SATA interrupts */
+
+	val = readl(tegra->sata_regs + SATA_INTR_MASK);
+	val |= SATA_INTR_MASK_IP_INT_MASK;
+	writel(val, tegra->sata_regs + SATA_INTR_MASK);
+
+	return 0;
+}
+
+static void tegra_ahci_controller_deinit(struct ahci_host_priv *hpriv)
+{
+	tegra_ahci_power_off(hpriv);
+}
+
+static void tegra_ahci_host_stop(struct ata_host *host)
+{
+	struct ahci_host_priv *hpriv = host->private_data;
+
+	tegra_ahci_controller_deinit(hpriv);
+}
+
+static struct ata_port_operations ahci_tegra_port_ops = {
+	.inherits	= &ahci_ops,
+	.host_stop	= tegra_ahci_host_stop,
+};
+
+static const struct ata_port_info ahci_tegra_port_info = {
+	.flags		= AHCI_FLAG_COMMON,
+	.pio_mask	= ATA_PIO4,
+	.udma_mask	= ATA_UDMA6,
+	.port_ops	= &ahci_tegra_port_ops,
+};
+
+static const struct of_device_id tegra_ahci_of_match[] = {
+	{ .compatible = "nvidia,tegra124-ahci" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, tegra_ahci_of_match);
+
+static int tegra_ahci_probe(struct platform_device *pdev)
+{
+	struct ahci_host_priv *hpriv;
+	struct tegra_ahci_priv *tegra;
+	struct resource *res;
+	int ret;
+
+	hpriv = ahci_platform_get_resources(pdev);
+	if (IS_ERR(hpriv))
+		return PTR_ERR(hpriv);
+
+	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
+	if (!tegra)
+		return -ENOMEM;
+
+	hpriv->plat_data = tegra;
+
+	tegra->pdev = pdev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	tegra->sata_regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(tegra->sata_regs))
+		return PTR_ERR(tegra->sata_regs);
+
+	tegra->sata_rst = devm_reset_control_get(&pdev->dev, "sata");
+	if (IS_ERR(tegra->sata_rst)) {
+		dev_err(&pdev->dev, "Failed to get sata reset\n");
+		return PTR_ERR(tegra->sata_rst);
+	}
+
+	tegra->sata_oob_rst = devm_reset_control_get(&pdev->dev, "sata-oob");
+	if (IS_ERR(tegra->sata_oob_rst)) {
+		dev_err(&pdev->dev, "Failed to get sata-oob reset\n");
+		return PTR_ERR(tegra->sata_oob_rst);
+	}
+
+	tegra->sata_cold_rst = devm_reset_control_get(&pdev->dev, "sata-cold");
+	if (IS_ERR(tegra->sata_cold_rst)) {
+		dev_err(&pdev->dev, "Failed to get sata-cold reset\n");
+		return PTR_ERR(tegra->sata_cold_rst);
+	}
+
+	tegra->sata_clk = devm_clk_get(&pdev->dev, "sata");
+	if (IS_ERR(tegra->sata_clk)) {
+		dev_err(&pdev->dev, "Failed to get sata clock\n");
+		return PTR_ERR(tegra->sata_clk);
+	}
+
+	tegra->supplies[0].supply = "avdd";
+	tegra->supplies[1].supply = "hvdd";
+	tegra->supplies[2].supply = "vddio";
+	tegra->supplies[3].supply = "target-5v";
+	tegra->supplies[4].supply = "target-12v";
+
+	ret = devm_regulator_bulk_get(&pdev->dev, ARRAY_SIZE(tegra->supplies),
+				      tegra->supplies);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to get regulators\n");
+		return ret;
+	}
+
+	ret = tegra_ahci_controller_init(hpriv);
+	if (ret)
+		return ret;
+
+	ret = ahci_platform_init_host(pdev, hpriv, &ahci_tegra_port_info,
+				      0, 0, 0);
+	if (ret)
+		goto deinit_controller;
+
+	return 0;
+
+deinit_controller:
+	tegra_ahci_controller_deinit(hpriv);
+
+	return ret;
+};
+
+static struct platform_driver tegra_ahci_driver = {
+	.probe = tegra_ahci_probe,
+	.remove = ata_platform_remove_one,
+	.driver = {
+		.name = "tegra-ahci",
+		.of_match_table = tegra_ahci_of_match,
+	},
+	/* LP0 suspend support not implemented */
+};
+module_platform_driver(tegra_ahci_driver);
+
+MODULE_AUTHOR("Mikko Perttunen <mperttunen@nvidia.com>");
+MODULE_DESCRIPTION("Tegra124 AHCI SATA driver");
+MODULE_LICENSE("GPL v2");
-- 
1.8.1.5

Re: [PATCH v5 1/8] of: Add NVIDIA Tegra SATA controller binding

[Mikko Perttunen]

So here's v5: this time, as suggested, I handle the sata clock myself 
and let ahci_platform handle it too, leading it to be prepared+enabled 
twice. This works fine, and allows us to remove the DT ordering requirement.

I also have in the works a patchset that adds the name-based 
ahci_platform_get_resources function, but that is not quite ready yet, 
even if it is quite far along. Also, I am going on vacation and 
returning on 28.7., so if this v5 is acceptable maybe it could be merged 
for 3.17 and I could work on the new get_resources scheme after I get 
back from vacation?

Thanks, Mikko

On 18/07/14 10:11, Mikko Perttunen wrote:
> This patch adds device tree binding documentation for the SATA
> controller found on NVIDIA Tegra SoCs.
>
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
> v5: remove ordering requirement again
>
>   .../devicetree/bindings/ata/tegra-sata.txt         | 30 ++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/ata/tegra-sata.txt
>
> diff --git a/Documentation/devicetree/bindings/ata/tegra-sata.txt b/Documentation/devicetree/bindings/ata/tegra-sata.txt
> new file mode 100644
> index 0000000..946f207
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ata/tegra-sata.txt
> @@ -0,0 +1,30 @@
> +Tegra124 SoC SATA AHCI controller
> +
> +Required properties :
> +- compatible : "nvidia,tegra124-ahci".
> +- reg : Should contain 2 entries:
> +  - AHCI register set (SATA BAR5)
> +  - SATA register set
> +- interrupts : Defines the interrupt used by SATA
> +- clocks : Must contain an entry for each entry in clock-names.
> +  See ../clocks/clock-bindings.txt for details.
> +- clock-names : Must include the following entries:
> +  - sata
> +  - sata-oob
> +  - cml1
> +  - pll_e
> +- resets : Must contain an entry for each entry in reset-names.
> +  See ../reset/reset.txt for details.
> +- reset-names : Must include the following entries:
> +  - sata
> +  - sata-oob
> +  - sata-cold
> +- phys : Must contain an entry for each entry in phy-names.
> +  See ../phy/phy-bindings.txt for details.
> +- phy-names : Must include the following entries:
> +  - sata-phy : XUSB PADCTL SATA PHY
> +- hvdd-supply : Defines the SATA HVDD regulator
> +- vddio-supply : Defines the SATA VDDIO regulator
> +- avdd-supply : Defines the SATA AVDD regulator
> +- target-5v-supply : Defines the SATA 5V power regulator
> +- target-12v-supply : Defines the SATA 12V power regulator
>

Re: [PATCH v5 7/8] ata: Add support for the Tegra124 SATA controller

[Hans de Goede]

Hi,

On 07/18/2014 09:12 AM, Mikko Perttunen wrote:
> This adds support for the integrated AHCI-compliant Serial ATA
> controller present on the NVIDIA Tegra124 system-on-chip.
> 
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
> v5: let ahci_platform handle sata clock and also handle it ourselves.
>   this allows use of ahci_platform while having a special sequence
>   for the clock.

Thanks, I like this one, much better then what we had before.

Acked-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> 
>  drivers/ata/Kconfig      |   9 ++
>  drivers/ata/Makefile     |   1 +
>  drivers/ata/ahci_tegra.c | 377 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 387 insertions(+)
>  create mode 100644 drivers/ata/ahci_tegra.c
> 
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index b0d5b5a..e1b9278 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -142,6 +142,15 @@ config AHCI_SUNXI
>  
>  	  If unsure, say N.
>  
> +config AHCI_TEGRA
> +	tristate "NVIDIA Tegra124 AHCI SATA support"
> +	depends on ARCH_TEGRA
> +	help
> +	  This option enables support for the NVIDIA Tegra124 SoC's
> +	  onboard AHCI SATA.
> +
> +	  If unsure, say N.
> +
>  config AHCI_XGENE
>  	tristate "APM X-Gene 6.0Gbps AHCI SATA host controller support"
>  	depends on PHY_XGENE
> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> index 5a02aee..ae41107 100644
> --- a/drivers/ata/Makefile
> +++ b/drivers/ata/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_AHCI_IMX)		+= ahci_imx.o libahci.o libahci_platform.o
>  obj-$(CONFIG_AHCI_MVEBU)	+= ahci_mvebu.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
> +obj-$(CONFIG_AHCI_TEGRA)	+= ahci_tegra.o libahci.o libahci_platform.o
>  obj-$(CONFIG_AHCI_XGENE)	+= ahci_xgene.o libahci.o libahci_platform.o
>  
>  # SFF w/ custom DMA
> diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c
> new file mode 100644
> index 0000000..d30bb21
> --- /dev/null
> +++ b/drivers/ata/ahci_tegra.c
> @@ -0,0 +1,377 @@
> +/*
> + * drivers/ata/ahci_tegra.c
> + *
> + * Copyright (c) 2014, NVIDIA CORPORATION.  All rights reserved.
> + *
> + * Author:
> + *	Mikko Perttunen <mperttunen@nvidia.com>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/ahci_platform.h>
> +#include <linux/reset.h>
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/tegra-powergate.h>
> +#include <linux/regulator/consumer.h>
> +#include "ahci.h"
> +
> +#define SATA_CONFIGURATION_0				0x180
> +#define SATA_CONFIGURATION_EN_FPCI			BIT(0)
> +
> +#define SCFG_OFFSET					0x1000
> +
> +#define T_SATA0_CFG_1					0x04
> +#define T_SATA0_CFG_1_IO_SPACE				BIT(0)
> +#define T_SATA0_CFG_1_MEMORY_SPACE			BIT(1)
> +#define T_SATA0_CFG_1_BUS_MASTER			BIT(2)
> +#define T_SATA0_CFG_1_SERR				BIT(8)
> +
> +#define T_SATA0_CFG_9					0x24
> +#define T_SATA0_CFG_9_BASE_ADDRESS_SHIFT		13
> +
> +#define SATA_FPCI_BAR5					0x94
> +#define SATA_FPCI_BAR5_START_SHIFT			4
> +
> +#define SATA_INTR_MASK					0x188
> +#define SATA_INTR_MASK_IP_INT_MASK			BIT(16)
> +
> +#define T_SATA0_AHCI_HBA_CAP_BKDR			0x300
> +
> +#define T_SATA0_BKDOOR_CC				0x4a4
> +
> +#define T_SATA0_CFG_SATA				0x54c
> +#define T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN		BIT(12)
> +
> +#define T_SATA0_CFG_MISC				0x550
> +
> +#define T_SATA0_INDEX					0x680
> +
> +#define T_SATA0_CHX_PHY_CTRL1_GEN1			0x690
> +#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK		0xff
> +#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT		0
> +#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK		(0xff << 8)
> +#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT	8
> +
> +#define T_SATA0_CHX_PHY_CTRL1_GEN2			0x694
> +#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_MASK		0xff
> +#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_SHIFT		0
> +#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_MASK		(0xff << 12)
> +#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_SHIFT	12
> +
> +#define T_SATA0_CHX_PHY_CTRL2				0x69c
> +#define T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1		0x23
> +
> +#define T_SATA0_CHX_PHY_CTRL11				0x6d0
> +#define T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ		(0x2800 << 16)
> +
> +#define FUSE_SATA_CALIB					0x124
> +#define FUSE_SATA_CALIB_MASK				0x3
> +
> +struct sata_pad_calibration {
> +	u8 gen1_tx_amp;
> +	u8 gen1_tx_peak;
> +	u8 gen2_tx_amp;
> +	u8 gen2_tx_peak;
> +};
> +
> +static const struct sata_pad_calibration tegra124_pad_calibration[] = {
> +	{0x18, 0x04, 0x18, 0x0a},
> +	{0x0e, 0x04, 0x14, 0x0a},
> +	{0x0e, 0x07, 0x1a, 0x0e},
> +	{0x14, 0x0e, 0x1a, 0x0e},
> +};
> +
> +struct tegra_ahci_priv {
> +	struct platform_device	   *pdev;
> +	void __iomem		   *sata_regs;
> +	struct reset_control	   *sata_rst;
> +	struct reset_control	   *sata_oob_rst;
> +	struct reset_control	   *sata_cold_rst;
> +	/* Needs special handling, cannot use ahci_platform */
> +	struct clk		   *sata_clk;
> +	struct regulator_bulk_data supplies[5];
> +};
> +
> +static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
> +{
> +	struct tegra_ahci_priv *tegra = hpriv->plat_data;
> +	int ret;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(tegra->supplies),
> +				    tegra->supplies);
> +	if (ret)
> +		return ret;
> +
> +	ret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_SATA,
> +						tegra->sata_clk,
> +						tegra->sata_rst);
> +	if (ret)
> +		goto disable_regulators;
> +
> +	reset_control_assert(tegra->sata_oob_rst);
> +	reset_control_assert(tegra->sata_cold_rst);
> +
> +	ret = ahci_platform_enable_resources(hpriv);
> +	if (ret)
> +		goto disable_power;
> +
> +	reset_control_deassert(tegra->sata_cold_rst);
> +	reset_control_deassert(tegra->sata_oob_rst);
> +
> +	return 0;
> +
> +disable_power:
> +	clk_disable_unprepare(tegra->sata_clk);
> +
> +	tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
> +
> +disable_regulators:
> +	regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra->supplies);
> +
> +	return ret;
> +}
> +
> +static void tegra_ahci_power_off(struct ahci_host_priv *hpriv)
> +{
> +	struct tegra_ahci_priv *tegra = hpriv->plat_data;
> +
> +	ahci_platform_disable_resources(hpriv);
> +
> +	reset_control_assert(tegra->sata_rst);
> +	reset_control_assert(tegra->sata_oob_rst);
> +	reset_control_assert(tegra->sata_cold_rst);
> +
> +	clk_disable_unprepare(tegra->sata_clk);
> +	tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
> +
> +	regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra->supplies);
> +}
> +
> +static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv)
> +{
> +	struct tegra_ahci_priv *tegra = hpriv->plat_data;
> +	int ret;
> +	unsigned int val;
> +	struct sata_pad_calibration calib;
> +
> +	ret = tegra_ahci_power_on(hpriv);
> +	if (ret) {
> +		dev_err(&tegra->pdev->dev,
> +			"failed to power on AHCI controller: %d\n", ret);
> +		return ret;
> +	}
> +
> +	val = readl(tegra->sata_regs + SATA_CONFIGURATION_0);
> +	val |= SATA_CONFIGURATION_EN_FPCI;
> +	writel(val, tegra->sata_regs + SATA_CONFIGURATION_0);
> +
> +	/* Pad calibration */
> +
> +	/* FIXME Always use calibration 0. Change this to read the calibration
> +	 * fuse once the fuse driver has landed. */
> +	val = 0;
> +
> +	calib = tegra124_pad_calibration[val & FUSE_SATA_CALIB_MASK];
> +
> +	writel(BIT(0), tegra->sata_regs + SCFG_OFFSET + T_SATA0_INDEX);
> +
> +	val = readl(tegra->sata_regs +
> +		SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN1);
> +	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK;
> +	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK;
> +	val |= calib.gen1_tx_amp <<
> +			T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT;
> +	val |= calib.gen1_tx_peak <<
> +			T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT;
> +	writel(val, tegra->sata_regs + SCFG_OFFSET +
> +		T_SATA0_CHX_PHY_CTRL1_GEN1);
> +
> +	val = readl(tegra->sata_regs +
> +			SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN2);
> +	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_MASK;
> +	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_MASK;
> +	val |= calib.gen2_tx_amp <<
> +			T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT;
> +	val |= calib.gen2_tx_peak <<
> +			T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT;
> +	writel(val, tegra->sata_regs + SCFG_OFFSET +
> +		T_SATA0_CHX_PHY_CTRL1_GEN2);
> +
> +	writel(T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ,
> +		tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL11);
> +	writel(T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1,
> +		tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL2);
> +
> +	writel(0, tegra->sata_regs + SCFG_OFFSET + T_SATA0_INDEX);
> +
> +	/* Program controller device ID */
> +
> +	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
> +	val |= T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN;
> +	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
> +
> +	writel(0x01060100, tegra->sata_regs + SCFG_OFFSET + T_SATA0_BKDOOR_CC);
> +
> +	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
> +	val &= ~T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN;
> +	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
> +
> +	/* Enable IO & memory access, bus master mode */
> +
> +	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_1);
> +	val |= T_SATA0_CFG_1_IO_SPACE | T_SATA0_CFG_1_MEMORY_SPACE |
> +		T_SATA0_CFG_1_BUS_MASTER | T_SATA0_CFG_1_SERR;
> +	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_1);
> +
> +	/* Program SATA MMIO */
> +
> +	writel(0x10000 << SATA_FPCI_BAR5_START_SHIFT,
> +	       tegra->sata_regs + SATA_FPCI_BAR5);
> +
> +	writel(0x08000 << T_SATA0_CFG_9_BASE_ADDRESS_SHIFT,
> +	       tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_9);
> +
> +	/* Unmask SATA interrupts */
> +
> +	val = readl(tegra->sata_regs + SATA_INTR_MASK);
> +	val |= SATA_INTR_MASK_IP_INT_MASK;
> +	writel(val, tegra->sata_regs + SATA_INTR_MASK);
> +
> +	return 0;
> +}
> +
> +static void tegra_ahci_controller_deinit(struct ahci_host_priv *hpriv)
> +{
> +	tegra_ahci_power_off(hpriv);
> +}
> +
> +static void tegra_ahci_host_stop(struct ata_host *host)
> +{
> +	struct ahci_host_priv *hpriv = host->private_data;
> +
> +	tegra_ahci_controller_deinit(hpriv);
> +}
> +
> +static struct ata_port_operations ahci_tegra_port_ops = {
> +	.inherits	= &ahci_ops,
> +	.host_stop	= tegra_ahci_host_stop,
> +};
> +
> +static const struct ata_port_info ahci_tegra_port_info = {
> +	.flags		= AHCI_FLAG_COMMON,
> +	.pio_mask	= ATA_PIO4,
> +	.udma_mask	= ATA_UDMA6,
> +	.port_ops	= &ahci_tegra_port_ops,
> +};
> +
> +static const struct of_device_id tegra_ahci_of_match[] = {
> +	{ .compatible = "nvidia,tegra124-ahci" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, tegra_ahci_of_match);
> +
> +static int tegra_ahci_probe(struct platform_device *pdev)
> +{
> +	struct ahci_host_priv *hpriv;
> +	struct tegra_ahci_priv *tegra;
> +	struct resource *res;
> +	int ret;
> +
> +	hpriv = ahci_platform_get_resources(pdev);
> +	if (IS_ERR(hpriv))
> +		return PTR_ERR(hpriv);
> +
> +	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
> +	if (!tegra)
> +		return -ENOMEM;
> +
> +	hpriv->plat_data = tegra;
> +
> +	tegra->pdev = pdev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	tegra->sata_regs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(tegra->sata_regs))
> +		return PTR_ERR(tegra->sata_regs);
> +
> +	tegra->sata_rst = devm_reset_control_get(&pdev->dev, "sata");
> +	if (IS_ERR(tegra->sata_rst)) {
> +		dev_err(&pdev->dev, "Failed to get sata reset\n");
> +		return PTR_ERR(tegra->sata_rst);
> +	}
> +
> +	tegra->sata_oob_rst = devm_reset_control_get(&pdev->dev, "sata-oob");
> +	if (IS_ERR(tegra->sata_oob_rst)) {
> +		dev_err(&pdev->dev, "Failed to get sata-oob reset\n");
> +		return PTR_ERR(tegra->sata_oob_rst);
> +	}
> +
> +	tegra->sata_cold_rst = devm_reset_control_get(&pdev->dev, "sata-cold");
> +	if (IS_ERR(tegra->sata_cold_rst)) {
> +		dev_err(&pdev->dev, "Failed to get sata-cold reset\n");
> +		return PTR_ERR(tegra->sata_cold_rst);
> +	}
> +
> +	tegra->sata_clk = devm_clk_get(&pdev->dev, "sata");
> +	if (IS_ERR(tegra->sata_clk)) {
> +		dev_err(&pdev->dev, "Failed to get sata clock\n");
> +		return PTR_ERR(tegra->sata_clk);
> +	}
> +
> +	tegra->supplies[0].supply = "avdd";
> +	tegra->supplies[1].supply = "hvdd";
> +	tegra->supplies[2].supply = "vddio";
> +	tegra->supplies[3].supply = "target-5v";
> +	tegra->supplies[4].supply = "target-12v";
> +
> +	ret = devm_regulator_bulk_get(&pdev->dev, ARRAY_SIZE(tegra->supplies),
> +				      tegra->supplies);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to get regulators\n");
> +		return ret;
> +	}
> +
> +	ret = tegra_ahci_controller_init(hpriv);
> +	if (ret)
> +		return ret;
> +
> +	ret = ahci_platform_init_host(pdev, hpriv, &ahci_tegra_port_info,
> +				      0, 0, 0);
> +	if (ret)
> +		goto deinit_controller;
> +
> +	return 0;
> +
> +deinit_controller:
> +	tegra_ahci_controller_deinit(hpriv);
> +
> +	return ret;
> +};
> +
> +static struct platform_driver tegra_ahci_driver = {
> +	.probe = tegra_ahci_probe,
> +	.remove = ata_platform_remove_one,
> +	.driver = {
> +		.name = "tegra-ahci",
> +		.of_match_table = tegra_ahci_of_match,
> +	},
> +	/* LP0 suspend support not implemented */
> +};
> +module_platform_driver(tegra_ahci_driver);
> +
> +MODULE_AUTHOR("Mikko Perttunen <mperttunen@nvidia.com>");
> +MODULE_DESCRIPTION("Tegra124 AHCI SATA driver");
> +MODULE_LICENSE("GPL v2");
> 

Re: [PATCH v5 1/8] of: Add NVIDIA Tegra SATA controller binding

[Hans de Goede]

Hi,

On 07/18/2014 09:11 AM, Mikko Perttunen wrote:
> This patch adds device tree binding documentation for the SATA
> controller found on NVIDIA Tegra SoCs.
> 
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
> v5: remove ordering requirement again

Acked-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> 
>  .../devicetree/bindings/ata/tegra-sata.txt         | 30 ++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ata/tegra-sata.txt
> 
> diff --git a/Documentation/devicetree/bindings/ata/tegra-sata.txt b/Documentation/devicetree/bindings/ata/tegra-sata.txt
> new file mode 100644
> index 0000000..946f207
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ata/tegra-sata.txt
> @@ -0,0 +1,30 @@
> +Tegra124 SoC SATA AHCI controller
> +
> +Required properties :
> +- compatible : "nvidia,tegra124-ahci".
> +- reg : Should contain 2 entries:
> +  - AHCI register set (SATA BAR5)
> +  - SATA register set
> +- interrupts : Defines the interrupt used by SATA
> +- clocks : Must contain an entry for each entry in clock-names.
> +  See ../clocks/clock-bindings.txt for details.
> +- clock-names : Must include the following entries:
> +  - sata
> +  - sata-oob
> +  - cml1
> +  - pll_e
> +- resets : Must contain an entry for each entry in reset-names.
> +  See ../reset/reset.txt for details.
> +- reset-names : Must include the following entries:
> +  - sata
> +  - sata-oob
> +  - sata-cold
> +- phys : Must contain an entry for each entry in phy-names.
> +  See ../phy/phy-bindings.txt for details.
> +- phy-names : Must include the following entries:
> +  - sata-phy : XUSB PADCTL SATA PHY
> +- hvdd-supply : Defines the SATA HVDD regulator
> +- vddio-supply : Defines the SATA VDDIO regulator
> +- avdd-supply : Defines the SATA AVDD regulator
> +- target-5v-supply : Defines the SATA 5V power regulator
> +- target-12v-supply : Defines the SATA 12V power regulator
> 

Re: [PATCH v5 1/8] of: Add NVIDIA Tegra SATA controller binding

[Hans de Goede]

Hi,

On 07/18/2014 09:16 AM, Mikko Perttunen wrote:
> So here's v5: this time, as suggested, I handle the sata clock myself and let ahci_platform handle it too, leading it to be prepared+enabled twice. This works fine, and allows us to remove the DT ordering requirement.
> 
> I also have in the works a patchset that adds the name-based ahci_platform_get_resources function, but that is not quite ready yet, even if it is quite far along. Also, I am going on vacation and returning on 28.7., so if this v5 is acceptable maybe it could be merged for 3.17 and I could work on the new get_resources scheme after I get back from vacation?

Yes that works for me v3 of all the patches with no newer version +
v5 of patch 1 and 7 are pretty clean and can go into 3.17 from my pov,
Tejun can you pick these up for 3.17 please?

And thanks for working on a name-based ahci_platform_get_resources function
since what we've now in v5 is quite clean already I think it will be good
to take our time to get this right, so postponing this to 3.18 is fine
with me.

Regards,

Hans

Re: [PATCH v5 7/8] ata: Add support for the Tegra124 SATA controller

[Mikko Perttunen]

Thanks!

- Mikko

On 18/07/14 13:26, Hans de Goede wrote:
> Hi,
>
> On 07/18/2014 09:12 AM, Mikko Perttunen wrote:
>> This adds support for the integrated AHCI-compliant Serial ATA
>> controller present on the NVIDIA Tegra124 system-on-chip.
>>
>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>> ---
>> v5: let ahci_platform handle sata clock and also handle it ourselves.
>>    this allows use of ahci_platform while having a special sequence
>>    for the clock.
>
> Thanks, I like this one, much better then what we had before.
>
> Acked-by: Hans de Goede <hdegoede@redhat.com>
>
> Regards,
>
> Hans
>
>
>
 >> ...

Re: [PATCH v5 1/8] of: Add NVIDIA Tegra SATA controller binding

[Mikko Perttunen]

On 18/07/14 13:28, Hans de Goede wrote:
> Hi,
>
> On 07/18/2014 09:16 AM, Mikko Perttunen wrote:
>> So here's v5: this time, as suggested, I handle the sata clock myself and let ahci_platform handle it too, leading it to be prepared+enabled twice. This works fine, and allows us to remove the DT ordering requirement.
>>
>> I also have in the works a patchset that adds the name-based ahci_platform_get_resources function, but that is not quite ready yet, even if it is quite far along. Also, I am going on vacation and returning on 28.7., so if this v5 is acceptable maybe it could be merged for 3.17 and I could work on the new get_resources scheme after I get back from vacation?
>
> Yes that works for me v3 of all the patches with no newer version +
> v5 of patch 1 and 7 are pretty clean and can go into 3.17 from my pov,
> Tejun can you pick these up for 3.17 please?

Note that the DTC step will fail if Thierry's xusb pinctrl series is not
applied before this one. (The DTS changes depend on a node called 
'padctl' and the pinctrl-tegra-xusb header)

>
> And thanks for working on a name-based ahci_platform_get_resources function
> since what we've now in v5 is quite clean already I think it will be good
> to take our time to get this right, so postponing this to 3.18 is fine
> with me.

Yes.

>
> Regards,
>
> Hans
>

Thanks, Mikko

Re: [PATCH v5 1/8] of: Add NVIDIA Tegra SATA controller binding

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1735 bytes --]

On Fri, Jul 18, 2014 at 12:28:54PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 07/18/2014 09:16 AM, Mikko Perttunen wrote:
> > So here's v5: this time, as suggested, I handle the sata clock myself and let ahci_platform handle it too, leading it to be prepared+enabled twice. This works fine, and allows us to remove the DT ordering requirement.
> > 
> > I also have in the works a patchset that adds the name-based ahci_platform_get_resources function, but that is not quite ready yet, even if it is quite far along. Also, I am going on vacation and returning on 28.7., so if this v5 is acceptable maybe it could be merged for 3.17 and I could work on the new get_resources scheme after I get back from vacation?
> 
> Yes that works for me v3 of all the patches with no newer version +
> v5 of patch 1 and 7 are pretty clean and can go into 3.17 from my pov,
> Tejun can you pick these up for 3.17 please?

Tejun,

I think the following patches can go through your tree without causing
breakage through unresolved dependencies:

	[PATCH v5 1/8] of: Add NVIDIA Tegra SATA controller binding
	[PATCH v3 6/8] ata: ahci_platform: Increase AHCI_MAX_CLKS to 4
	[PATCH v5 7/8] ata: Add support for the Tegra124 SATA controller

Peter, you should probably pick up

	[PATCH v3 4/8] clk: tegra: Enable hardware control of SATA PLL
	[PATCH v3 5/8] clk: tegra: Add SATA clocks to Tegra124 initialization table

and send them off to Mike for 3.17 if there's still time.

Stephen, the rest of the patches would probably best go through the
Tegra tree so that we can handle the dependencies there. I've already
sent out pull requests for 3.17 today, but maybe Arnd and Olof can be
convinced to take one more.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v5 1/8] of: Add NVIDIA Tegra SATA controller binding

[Tejun Heo]

On Fri, Jul 18, 2014 at 05:06:04PM +0200, Thierry Reding wrote:
> I think the following patches can go through your tree without causing
> breakage through unresolved dependencies:
> 
> 	[PATCH v5 1/8] of: Add NVIDIA Tegra SATA controller binding
> 	[PATCH v3 6/8] ata: ahci_platform: Increase AHCI_MAX_CLKS to 4
> 	[PATCH v5 7/8] ata: Add support for the Tegra124 SATA controller

Applied to libata/for-3.17 w/ Hans' acks added.  Please check the
following branch and give me a holler if something is amiss.

 git://git.kernel.org/pub/scm/linux/kernel/git/tj/libata.git for-3.17

Thanks.

-- 
tejun

Re: [PATCH v5 1/8] of: Add NVIDIA Tegra SATA controller binding

[Stephen Warren]

[-- Attachment #1: Type: text/plain, Size: 1455 bytes --]

On 07/18/2014 09:06 AM, Thierry Reding wrote:
> On Fri, Jul 18, 2014 at 12:28:54PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 07/18/2014 09:16 AM, Mikko Perttunen wrote:
>>> So here's v5: this time, as suggested, I handle the sata clock myself and let ahci_platform handle it too, leading it to be prepared+enabled twice. This works fine, and allows us to remove the DT ordering requirement.
>>>
>>> I also have in the works a patchset that adds the name-based ahci_platform_get_resources function, but that is not quite ready yet, even if it is quite far along. Also, I am going on vacation and returning on 28.7., so if this v5 is acceptable maybe it could be merged for 3.17 and I could work on the new get_resources scheme after I get back from vacation?
>>
>> Yes that works for me v3 of all the patches with no newer version +
>> v5 of patch 1 and 7 are pretty clean and can go into 3.17 from my pov,
>> Tejun can you pick these up for 3.17 please?
> 
> Tejun,
> 
> I think the following patches can go through your tree without causing
> breakage through unresolved dependencies:
...
> Stephen, the rest of the patches would probably best go through the
> Tegra tree so that we can handle the dependencies there. I've already
> sent out pull requests for 3.17 today, but maybe Arnd and Olof can be
> convinced to take one more.

At this point, I'm just going to wait for 3.18 for anything that didn't
make it into 3.17.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH v3 2/8] ARM: tegra: Add SATA controller to Tegra124 device tree

[Stephen Warren]

On 07/16/2014 02:54 AM, Mikko Perttunen wrote:
> This adds the integrated AHCI-compliant Serial ATA controller present
> in Tegra124 systems-on-chip to the Tegra124 device tree.

I have applied patches 2 and 3 to Tegra's for-3.18/dt branch. I fixed 
the DT node sort order when doing so.

I can't apply patch 8 (the defconfig change) until 3.17-rc2 is out, or 
there will be a build failure, but I will do so ASAP.

BTW, once 3.17-rc2 is out, can you please sent a patch to 
multi_v7_defconfig enabling Tegra SATA support there too. Thanks.

Re: [PATCH v3 2/8] ARM: tegra: Add SATA controller to Tegra124 device tree

[Mikko Perttunen]

On 25/08/14 20:23, Stephen Warren wrote:
> On 07/16/2014 02:54 AM, Mikko Perttunen wrote:
>> This adds the integrated AHCI-compliant Serial ATA controller present
>> in Tegra124 systems-on-chip to the Tegra124 device tree.
>
> I have applied patches 2 and 3 to Tegra's for-3.18/dt branch. I fixed
> the DT node sort order when doing so.

Thanks :)

>
> I can't apply patch 8 (the defconfig change) until 3.17-rc2 is out, or
> there will be a build failure, but I will do so ASAP.
>
> BTW, once 3.17-rc2 is out, can you please sent a patch to
> multi_v7_defconfig enabling Tegra SATA support there too. Thanks.

Will do.

Cheers,
Mikko

Re: [PATCH v3 8/8] ARM: tegra: Add options for Tegra AHCI support to tegra_defconfig

[Stephen Warren]

On 07/16/2014 02:54 AM, Mikko Perttunen wrote:
> This adds ATA, SATA_AHCI and AHCI_TEGRA support to tegra_defconfig
> so that the SATA support will be automatically enabled.

I've applied this to Tegra's for-3.18/dt branch.