[PATCH v2 0/3] ADD AHCI support for tegra210

[PATCH v2 0/3] ADD AHCI support for tegra210

[Preetham Chandru Ramchandra]

From: Preetham Chandru R <pchandru@nvidia.com>

1. ADD AHCI support for tegra210
2. Add support to disable DIPM, HIPM, DevSlp,
   partial, slumber, NCQ features from DT.
3. Extend the tegra AHCI controller device tree binding with tegra210
---
Preetham Chandru R (3):
  ata: ahci_tegra: add support for tegra210
  ata: ahci_tegra: Add support to disable features
  dt-bindings: ata: ahci_tegra: Add tegra210 AHCI support

 .../bindings/ata/nvidia,tegra124-ahci.txt          |  48 +-
 drivers/ata/ahci_tegra.c                           | 581 ++++++++++++++++-----
 2 files changed, 483 insertions(+), 146 deletions(-)

-- 
2.1.4

[PATCH v2 1/3] ata: ahci_tegra: add support for tegra210

[Preetham Chandru Ramchandra]

From: Preetham Chandru R <pchandru@nvidia.com>

Add AHCI support for tegra210
1. Moved tegra124 specifics to tegra124_ahci_init.
2. Separated out the regulators needed for tegra124 and tegra210.
3. Set the LPM capabilities
4. Create inline functions for read/write and modify to
   SATA, SATA Config and SATA Aux registers.

Signed-off-by: Preetham Chandru R <pchandru@nvidia.com>
---
v2:
* Fixed indentation issues
* Moved the change to disable DIPM, HIPM, DevSlp, partial,
  slumber and NCQ into a separate patch

 drivers/ata/ahci_tegra.c | 478 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 348 insertions(+), 130 deletions(-)

diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c
index 3a62eb2..d12e2a9 100644
--- a/drivers/ata/ahci_tegra.c
+++ b/drivers/ata/ahci_tegra.c
@@ -33,32 +33,74 @@
 
 #define DRV_NAME "tegra-ahci"
 
+#define SATA_FPCI_BAR5_0				0x94
+#define FPCI_BAR5_START_MASK				(0xFFFFFFF << 4)
+#define FPCI_BAR5_START					(0x0040020 << 4)
+#define FPCI_BAR5_ACCESS_TYPE				(0x1)
+
 #define SATA_CONFIGURATION_0				0x180
-#define SATA_CONFIGURATION_EN_FPCI			BIT(0)
+#define SATA_CONFIGURATION_0_EN_FPCI			BIT(0)
+#define SATA_CONFIGURATION_CLK_OVERRIDE			BIT(31)
+
+#define SATA_INTR_MASK_0				0x188
+#define IP_INT_MASK					BIT(16)
 
 #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_SATA_CFG_1					0x4
+#define T_SATA_CFG_1_IO_SPACE				BIT(0)
+#define T_SATA_CFG_1_MEMORY_SPACE			BIT(1)
+#define T_SATA_CFG_1_BUS_MASTER				BIT(2)
+#define T_SATA_CFG_1_SERR				BIT(8)
+
+#define T_SATA_CFG_9					0x24
+#define T_SATA_CFG_9_BASE_ADDRESS			0x40020000
+
+#define T_SATA0_CFG_35					0x94
+#define T_SATA0_CFG_35_IDP_INDEX_MASK			(0x7FF << 2)
+#define T_SATA0_CFG_35_IDP_INDEX			(0x2A << 2)
 
-#define T_SATA0_CFG_9					0x24
-#define T_SATA0_CFG_9_BASE_ADDRESS_SHIFT		13
+#define T_SATA0_AHCI_IDP1				0x98
+#define T_SATA0_AHCI_IDP1_DATA				(0x400040)
 
-#define SATA_FPCI_BAR5					0x94
-#define SATA_FPCI_BAR5_START_SHIFT			4
+#define T_SATA0_CFG_PHY_1				0x12C
+#define T_SATA0_CFG_PHY_1_PADS_IDDQ_EN			BIT(23)
+#define T_SATA0_CFG_PHY_1_PAD_PLL_IDDQ_EN		BIT(22)
 
-#define SATA_INTR_MASK					0x188
-#define SATA_INTR_MASK_IP_INT_MASK			BIT(16)
+#define T_SATA0_NVOOB                                   0x114
+#define T_SATA0_NVOOB_COMMA_CNT_MASK                    (0xff << 16)
+#define T_SATA0_NVOOB_COMMA_CNT                         (0x07 << 16)
+#define T_SATA0_NVOOB_SQUELCH_FILTER_MODE_MASK          (0x3 << 24)
+#define T_SATA0_NVOOB_SQUELCH_FILTER_MODE               (0x1 << 24)
+#define T_SATA0_NVOOB_SQUELCH_FILTER_LENGTH_MASK        (0x3 << 26)
+#define T_SATA0_NVOOB_SQUELCH_FILTER_LENGTH             (0x3 << 26)
+
+#define T_SATA_CFG_PHY_0                                0x120
+#define T_SATA_CFG_PHY_0_USE_7BIT_ALIGN_DET_FOR_SPD     BIT(11)
+#define T_SATA_CFG_PHY_0_MASK_SQUELCH                   BIT(24)
+
+#define FUSE_SATA_CALIB					0x124
+#define FUSE_SATA_CALIB_MASK				0x3
+
+#define T_SATA0_CFG2NVOOB_2				0x134
+#define T_SATA0_CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW_MASK	(0x1ff << 18)
+#define T_SATA0_CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW	(0xc << 18)
 
 #define T_SATA0_AHCI_HBA_CAP_BKDR			0x300
+#define T_SATA0_AHCI_HBA_CAP_BKDR_PARTIAL_ST_CAP	BIT(13)
+#define T_SATA0_AHCI_HBA_CAP_BKDR_SLUMBER_ST_CAP	BIT(14)
+#define T_SATA0_AHCI_HBA_CAP_BKDR_SALP			BIT(26)
+#define T_SATA0_AHCI_HBA_CAP_BKDR_SUPP_PM		BIT(17)
+#define T_SATA0_AHCI_HBA_CAP_BKDR_SNCQ			BIT(30)
 
-#define T_SATA0_BKDOOR_CC				0x4a4
+#define T_SATA_BKDOOR_CC				0x4A4
+#define T_SATA_BKDOOR_CC_CLASS_CODE_MASK		(0xFFFF << 16)
+#define T_SATA_BKDOOR_CC_CLASS_CODE			(0x0106 << 16)
+#define T_SATA_BKDOOR_CC_PROG_IF_MASK			(0xFF << 8)
+#define T_SATA_BKDOOR_CC_PROG_IF			(0x01 << 8)
 
-#define T_SATA0_CFG_SATA				0x54c
-#define T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN		BIT(12)
+#define T_SATA_CFG_SATA					0x54C
+#define T_SATA_CFG_SATA_BACKDOOR_PROG_IF_EN		BIT(12)
 
 #define T_SATA0_CFG_MISC				0x550
 
@@ -82,8 +124,27 @@
 #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
+/* Electrical settings for better link stability */
+#define T_SATA0_CHX_PHY_CTRL17_0			0x6e8
+#define T_SATA0_CHX_PHY_CTRL17_0_RX_EQ_CTRL_L_GEN1	0x55010000
+#define T_SATA0_CHX_PHY_CTRL18_0			0x6ec
+#define T_SATA0_CHX_PHY_CTRL18_0_RX_EQ_CTRL_L_GEN2	0x55010000
+#define T_SATA0_CHX_PHY_CTRL20_0			0x6f4
+#define T_SATA0_CHX_PHY_CTRL20_0_RX_EQ_CTRL_H_GEN1	0x1
+#define T_SATA0_CHX_PHY_CTRL21_0			0x6f8
+#define T_SATA0_CHX_PHY_CTRL21_0_RX_EQ_CTRL_H_GEN2	0x1
+
+/* AUX Registers */
+#define SATA_AUX_MISC_CNTL_1_0				0x8
+#define DEVSLP_OVERRIDE					BIT(17)
+#define SDS_SUPPORT					BIT(13)
+#define DESO_SUPPORT					BIT(15)
+
+#define SATA_AUX_RX_STAT_INT_0				0xc
+#define SATA_DEVSLP					BIT(7)
+
+#define SATA_AUX_SPARE_CFG0_0				0x18
+#define MDAT_TIMER_AFTER_PG_VALID			BIT(14)
 
 struct sata_pad_calibration {
 	u8 gen1_tx_amp;
@@ -99,15 +160,135 @@ static const struct sata_pad_calibration tegra124_pad_calibration[] = {
 	{0x14, 0x0e, 0x1a, 0x0e},
 };
 
+struct tegra_ahci_ops {
+	int (*init)(struct ahci_host_priv *);
+};
+
+struct tegra_ahci_soc {
+	const char *const *supply_names;
+	unsigned int num_supplies;
+	struct tegra_ahci_ops ops;
+};
+
 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 platform_device *pdev;
+	void __iomem *sata_regs;
+	void __iomem *sata_aux_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];
+	struct clk *sata_clk;
+	struct regulator_bulk_data *supplies;
+	struct tegra_ahci_soc *soc_data;
+};
+
+static const char *const tegra124_supply_names[] = {
+	"avdd", "hvdd", "vddio", "target-5v", "target-12v"
+};
+
+static inline void tegra_ahci_sata_update(struct tegra_ahci_priv *tegra,
+					  u32 val, u32 mask, u32 offset)
+{
+	u32 uval;
+
+	uval = readl(tegra->sata_regs + offset);
+	uval = (uval & ~mask) | (val & mask);
+	writel(uval, tegra->sata_regs + offset);
+}
+
+static inline void tegra_ahci_scfg_writel(struct tegra_ahci_priv *tegra,
+					  u32 val, u32 offset)
+{
+	writel(val, tegra->sata_regs + SCFG_OFFSET + offset);
+}
+
+static inline void tegra_ahci_scfg_update(struct tegra_ahci_priv *tegra,
+					  u32 val, u32 mask, u32 offset)
+{
+	u32 uval;
+
+	uval = readl(tegra->sata_regs + SCFG_OFFSET + offset);
+	uval = (uval & ~mask) | (val & mask);
+	writel(uval, tegra->sata_regs + SCFG_OFFSET + offset);
+}
+
+static inline u32 tegra_ahci_aux_readl(struct tegra_ahci_priv *tegra,
+				       u32 offset)
+{
+	return readl(tegra->sata_aux_regs + offset);
+}
+
+static inline void tegra_ahci_aux_update(struct tegra_ahci_priv *tegra, u32 val,
+					 u32 mask, u32 offset)
+{
+	u32 uval;
+
+	uval = readl(tegra->sata_aux_regs + offset);
+	uval = (uval & ~mask) | (val & mask);
+	writel(uval, tegra->sata_aux_regs + offset);
+}
+
+static int tegra124_ahci_init(struct ahci_host_priv *hpriv)
+{
+	struct tegra_ahci_priv *tegra = hpriv->plat_data;
+	struct sata_pad_calibration calib;
+	int ret;
+	u32 val;
+	u32 mask;
+
+	/* Pad calibration */
+	ret = tegra_fuse_readl(FUSE_SATA_CALIB, &val);
+	if (ret)
+		return ret;
+
+	calib = tegra124_pad_calibration[val & FUSE_SATA_CALIB_MASK];
+
+	tegra_ahci_scfg_writel(tegra, BIT(0), T_SATA0_INDEX);
+
+	mask = T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK |
+	    T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK;
+	val = (calib.gen1_tx_amp << T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT) |
+	    (calib.gen1_tx_peak << T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT);
+	tegra_ahci_scfg_update(tegra, val, mask, T_SATA0_CHX_PHY_CTRL1_GEN1);
+
+	mask = T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_MASK |
+	    T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_MASK;
+	val = (calib.gen2_tx_amp << T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT) |
+	    (calib.gen2_tx_peak << T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT);
+	tegra_ahci_scfg_update(tegra, val, mask, T_SATA0_CHX_PHY_CTRL1_GEN2);
+
+	tegra_ahci_scfg_writel(tegra,
+			       T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ,
+			       T_SATA0_CHX_PHY_CTRL11);
+	tegra_ahci_scfg_writel(tegra,
+			       T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1,
+			       T_SATA0_CHX_PHY_CTRL2);
+
+	tegra_ahci_scfg_writel(tegra, 0, T_SATA0_INDEX);
+
+	return 0;
+}
+
+static const struct tegra_ahci_soc tegra124_ahci_soc_data = {
+	.supply_names = tegra124_supply_names,
+	.num_supplies = ARRAY_SIZE(tegra124_supply_names),
+	.ops = {
+		.init = tegra124_ahci_init,
+		},
+};
+
+static const char *const tegra210_supply_names[] = {
+	"dvdd-sata-pll",
+	"hvdd-sata",
+	"l0-hvddio-sata",
+	"l0-dvddio-sata",
+	"hvdd-pex-pll-e"
+};
+
+static const struct tegra_ahci_soc tegra210_ahci_soc_data = {
+	.supply_names = tegra210_supply_names,
+	.num_supplies = ARRAY_SIZE(tegra210_supply_names),
 };
 
 static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
@@ -115,7 +296,7 @@ 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),
+	ret = regulator_bulk_enable(tegra->soc_data->num_supplies,
 				    tegra->supplies);
 	if (ret)
 		return ret;
@@ -144,8 +325,7 @@ static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
 	tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
 
 disable_regulators:
-	regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra->supplies);
-
+	regulator_bulk_disable(tegra->soc_data->num_supplies, tegra->supplies);
 	return ret;
 }
 
@@ -162,101 +342,124 @@ static void tegra_ahci_power_off(struct ahci_host_priv *hpriv)
 	clk_disable_unprepare(tegra->sata_clk);
 	tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
 
-	regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra->supplies);
+	regulator_bulk_disable(tegra->soc_data->num_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;
+	u32 val;
+	u32 mask;
 
 	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 */
-
-	ret = tegra_fuse_readl(FUSE_SATA_CALIB, &val);
-	if (ret) {
-		dev_err(&tegra->pdev->dev,
-			"failed to read calibration fuse: %d\n", ret);
+	if (ret)
 		return ret;
-	}
-
-	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);
+	/*
+	 * Program the following  SATA IPFS registers
+	 * to allow SW accesses to SATA's MMIO Register
+	 */
+	mask = FPCI_BAR5_START_MASK | FPCI_BAR5_ACCESS_TYPE;
+	val = FPCI_BAR5_START | FPCI_BAR5_ACCESS_TYPE;
+	tegra_ahci_sata_update(tegra, val, mask, SATA_FPCI_BAR5_0);
+
+	/* Program the following SATA IPFS register to enable the SATA */
+	val = SATA_CONFIGURATION_0_EN_FPCI;
+	tegra_ahci_sata_update(tegra, val, val, SATA_CONFIGURATION_0);
+
+	/* Electrical settings for better link stability */
+	tegra_ahci_scfg_writel(tegra,
+			       T_SATA0_CHX_PHY_CTRL17_0_RX_EQ_CTRL_L_GEN1,
+			       T_SATA0_CHX_PHY_CTRL17_0);
+	tegra_ahci_scfg_writel(tegra,
+			       T_SATA0_CHX_PHY_CTRL18_0_RX_EQ_CTRL_L_GEN2,
+			       T_SATA0_CHX_PHY_CTRL18_0);
+	tegra_ahci_scfg_writel(tegra,
+			       T_SATA0_CHX_PHY_CTRL20_0_RX_EQ_CTRL_H_GEN1,
+			       T_SATA0_CHX_PHY_CTRL20_0);
+	tegra_ahci_scfg_writel(tegra,
+			       T_SATA0_CHX_PHY_CTRL21_0_RX_EQ_CTRL_H_GEN2,
+			       T_SATA0_CHX_PHY_CTRL21_0);
+
+	/* For SQUELCH Filter & Gen3 drive getting detected as Gen1 drive */
+
+	mask = T_SATA_CFG_PHY_0_MASK_SQUELCH |
+	    T_SATA_CFG_PHY_0_USE_7BIT_ALIGN_DET_FOR_SPD;
+	val = T_SATA_CFG_PHY_0_MASK_SQUELCH;
+	val &= ~T_SATA_CFG_PHY_0_USE_7BIT_ALIGN_DET_FOR_SPD;
+	tegra_ahci_scfg_update(tegra, val, mask, T_SATA_CFG_PHY_0);
+
+	mask = (T_SATA0_NVOOB_COMMA_CNT_MASK |
+		T_SATA0_NVOOB_SQUELCH_FILTER_LENGTH_MASK |
+		T_SATA0_NVOOB_SQUELCH_FILTER_MODE_MASK);
+	val = (T_SATA0_NVOOB_COMMA_CNT |
+	       T_SATA0_NVOOB_SQUELCH_FILTER_LENGTH |
+	       T_SATA0_NVOOB_SQUELCH_FILTER_MODE);
+	tegra_ahci_scfg_update(tegra, val, mask, T_SATA0_NVOOB);
+
+	/*
+	 * Change CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW from 83.3 ns to 58.8ns
+	 */
+	mask = T_SATA0_CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW_MASK;
+	val = T_SATA0_CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW;
+	tegra_ahci_scfg_update(tegra, val, mask, T_SATA0_CFG2NVOOB_2);
+
+	if (tegra->soc_data->ops.init)
+		tegra->soc_data->ops.init(hpriv);
+
+	/*
+	 * Program the following SATA configuration registers
+	 * to initialize SATA
+	 */
+	val = (T_SATA_CFG_1_IO_SPACE | T_SATA_CFG_1_MEMORY_SPACE |
+	       T_SATA_CFG_1_BUS_MASTER | T_SATA_CFG_1_SERR);
+	tegra_ahci_scfg_update(tegra, val, val, T_SATA_CFG_1);
+	tegra_ahci_scfg_writel(tegra, T_SATA_CFG_9_BASE_ADDRESS, T_SATA_CFG_9);
+
+	/* Program Class Code and Programming interface for SATA */
+	val = T_SATA_CFG_SATA_BACKDOOR_PROG_IF_EN;
+	tegra_ahci_scfg_update(tegra, val, val, T_SATA_CFG_SATA);
+
+	mask = T_SATA_BKDOOR_CC_CLASS_CODE_MASK | T_SATA_BKDOOR_CC_PROG_IF_MASK;
+	val = T_SATA_BKDOOR_CC_CLASS_CODE | T_SATA_BKDOOR_CC_PROG_IF;
+	tegra_ahci_scfg_update(tegra, val, mask, T_SATA_BKDOOR_CC);
+
+	tegra_ahci_scfg_update(tegra, 0, T_SATA_CFG_SATA_BACKDOOR_PROG_IF_EN,
+			       T_SATA_CFG_SATA);
+
+	/* Enabling LPM capabilities through Backdoor Programming */
+	val = (T_SATA0_AHCI_HBA_CAP_BKDR_PARTIAL_ST_CAP |
+	       T_SATA0_AHCI_HBA_CAP_BKDR_SLUMBER_ST_CAP |
+	       T_SATA0_AHCI_HBA_CAP_BKDR_SALP |
+	       T_SATA0_AHCI_HBA_CAP_BKDR_SUPP_PM);
+	tegra_ahci_scfg_update(tegra, val, val, T_SATA0_AHCI_HBA_CAP_BKDR);
+
+	/* SATA Second Level Clock Gating configuration
+	 * Enabling Gating of Tx/Rx clocks and driving Pad IDDQ and Lane
+	 * IDDQ Signals
+	 */
+	mask = T_SATA0_CFG_35_IDP_INDEX_MASK;
+	val = T_SATA0_CFG_35_IDP_INDEX;
+	tegra_ahci_scfg_update(tegra, val, mask, T_SATA0_CFG_35);
+	tegra_ahci_scfg_writel(tegra, T_SATA0_AHCI_IDP1_DATA,
+			       T_SATA0_AHCI_IDP1);
+	val = (T_SATA0_CFG_PHY_1_PADS_IDDQ_EN |
+	       T_SATA0_CFG_PHY_1_PAD_PLL_IDDQ_EN);
+	tegra_ahci_scfg_update(tegra, val, val, T_SATA0_CFG_PHY_1);
+
+	/*
+	 *  Indicate Sata only has the capability to enter DevSleep
+	 * from slumber link.
+	 */
+	tegra_ahci_aux_update(tegra, DESO_SUPPORT, DESO_SUPPORT,
+			      SATA_AUX_MISC_CNTL_1_0);
+	/* Enabling IPFS Clock Gating */
+	tegra_ahci_sata_update(tegra, 0, SATA_CONFIGURATION_CLK_OVERRIDE,
+			       SATA_CONFIGURATION_0);
+
+	tegra_ahci_sata_update(tegra, IP_INT_MASK, IP_INT_MASK,
+			       SATA_INTR_MASK_0);
 
 	return 0;
 }
@@ -274,19 +477,24 @@ static void tegra_ahci_host_stop(struct ata_host *host)
 }
 
 static struct ata_port_operations ahci_tegra_port_ops = {
-	.inherits	= &ahci_ops,
-	.host_stop	= tegra_ahci_host_stop,
+	.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,
+	.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" },
+	{
+	 .compatible = "nvidia,tegra124-ahci",
+	 .data = &tegra124_ahci_soc_data},
+	{
+	 .compatible = "nvidia,tegra210-ahci",
+	 .data = &tegra210_ahci_soc_data},
 	{}
 };
 MODULE_DEVICE_TABLE(of, tegra_ahci_of_match);
@@ -301,6 +509,7 @@ static int tegra_ahci_probe(struct platform_device *pdev)
 	struct tegra_ahci_priv *tegra;
 	struct resource *res;
 	int ret;
+	unsigned int i;
 
 	hpriv = ahci_platform_get_resources(pdev);
 	if (IS_ERR(hpriv))
@@ -311,13 +520,18 @@ static int tegra_ahci_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	hpriv->plat_data = tegra;
-
 	tegra->pdev = pdev;
+	tegra->soc_data =
+	    (struct tegra_ahci_soc *)of_device_get_match_data(&pdev->dev);
 
 	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);
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
+	tegra->sata_aux_regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(tegra->sata_aux_regs))
+		return PTR_ERR(tegra->sata_aux_regs);
 
 	tegra->sata_rst = devm_reset_control_get(&pdev->dev, "sata");
 	if (IS_ERR(tegra->sata_rst)) {
@@ -343,13 +557,17 @@ static int tegra_ahci_probe(struct platform_device *pdev)
 		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";
+	tegra->supplies = devm_kcalloc(&pdev->dev,
+				       tegra->soc_data->num_supplies,
+				       sizeof(*tegra->supplies), GFP_KERNEL);
+	if (!tegra->supplies)
+		return -ENOMEM;
+
+	for (i = 0; i < tegra->soc_data->num_supplies; i++)
+		tegra->supplies[i].supply = tegra->soc_data->supply_names[i];
 
-	ret = devm_regulator_bulk_get(&pdev->dev, ARRAY_SIZE(tegra->supplies),
+	ret = devm_regulator_bulk_get(&pdev->dev,
+				      tegra->soc_data->num_supplies,
 				      tegra->supplies);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to get regulators\n");
@@ -377,13 +595,13 @@ static struct platform_driver tegra_ahci_driver = {
 	.probe = tegra_ahci_probe,
 	.remove = ata_platform_remove_one,
 	.driver = {
-		.name = DRV_NAME,
-		.of_match_table = tegra_ahci_of_match,
-	},
+		   .name = DRV_NAME,
+		   .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_DESCRIPTION("Tegra AHCI SATA driver");
 MODULE_LICENSE("GPL v2");
-- 
2.1.4

Re: [v2,1/3] ata: ahci_tegra: add support for tegra210

[Mikko Perttunen]

+Cc linux-tegra

Hi Preetham,

I'll do a review pass. Please also Cc the next version to 
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org so that more Tegra developers have visibility.

On 24.11.2016 09:43, PREETHAM RAMACHANDRA wrote:
> From: Preetham Chandru R <pchandru-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> Add AHCI support for tegra210
> 1. Moved tegra124 specifics to tegra124_ahci_init.
> 2. Separated out the regulators needed for tegra124 and tegra210.
> 3. Set the LPM capabilities
> 4. Create inline functions for read/write and modify to
>    SATA, SATA Config and SATA Aux registers.
>
> Signed-off-by: Preetham Chandru R <pchandru-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> v2:
> * Fixed indentation issues
> * Moved the change to disable DIPM, HIPM, DevSlp, partial,
>   slumber and NCQ into a separate patch
>
>  drivers/ata/ahci_tegra.c | 478 ++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 348 insertions(+), 130 deletions(-)
>
> diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c
> index 3a62eb2..d12e2a9 100644
> --- a/drivers/ata/ahci_tegra.c
> +++ b/drivers/ata/ahci_tegra.c
> @@ -33,32 +33,74 @@
>
>  #define DRV_NAME "tegra-ahci"
>
> +#define SATA_FPCI_BAR5_0				0x94
> +#define FPCI_BAR5_START_MASK				(0xFFFFFFF << 4)
> +#define FPCI_BAR5_START					(0x0040020 << 4)
> +#define FPCI_BAR5_ACCESS_TYPE				(0x1)

Please use the existing convention in this file, that is, fields are 
prefixed with the register name. Also, please use small letters for hex 
digits as used in the file elsewhere.

> +
>  #define SATA_CONFIGURATION_0				0x180
> -#define SATA_CONFIGURATION_EN_FPCI			BIT(0)
> +#define SATA_CONFIGURATION_0_EN_FPCI			BIT(0)
> +#define SATA_CONFIGURATION_CLK_OVERRIDE			BIT(31)
> +
> +#define SATA_INTR_MASK_0				0x188
> +#define IP_INT_MASK					BIT(16)
>
>  #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_SATA_CFG_1					0x4
> +#define T_SATA_CFG_1_IO_SPACE				BIT(0)
> +#define T_SATA_CFG_1_MEMORY_SPACE			BIT(1)
> +#define T_SATA_CFG_1_BUS_MASTER				BIT(2)
> +#define T_SATA_CFG_1_SERR				BIT(8)

Try not to rename and move fields unnecessarily. It makes it very 
difficult to see where things have actually changed and makes the patch 
unnecessarily long.

> +
> +#define T_SATA_CFG_9					0x24
> +#define T_SATA_CFG_9_BASE_ADDRESS			0x40020000
> +
> +#define T_SATA0_CFG_35					0x94
> +#define T_SATA0_CFG_35_IDP_INDEX_MASK			(0x7FF << 2)
> +#define T_SATA0_CFG_35_IDP_INDEX			(0x2A << 2)
>
> -#define T_SATA0_CFG_9					0x24
> -#define T_SATA0_CFG_9_BASE_ADDRESS_SHIFT		13
> +#define T_SATA0_AHCI_IDP1				0x98
> +#define T_SATA0_AHCI_IDP1_DATA				(0x400040)
>
> -#define SATA_FPCI_BAR5					0x94
> -#define SATA_FPCI_BAR5_START_SHIFT			4
> +#define T_SATA0_CFG_PHY_1				0x12C
> +#define T_SATA0_CFG_PHY_1_PADS_IDDQ_EN			BIT(23)
> +#define T_SATA0_CFG_PHY_1_PAD_PLL_IDDQ_EN		BIT(22)
>
> -#define SATA_INTR_MASK					0x188
> -#define SATA_INTR_MASK_IP_INT_MASK			BIT(16)
> +#define T_SATA0_NVOOB                                   0x114
> +#define T_SATA0_NVOOB_COMMA_CNT_MASK                    (0xff << 16)
> +#define T_SATA0_NVOOB_COMMA_CNT                         (0x07 << 16)
> +#define T_SATA0_NVOOB_SQUELCH_FILTER_MODE_MASK          (0x3 << 24)
> +#define T_SATA0_NVOOB_SQUELCH_FILTER_MODE               (0x1 << 24)
> +#define T_SATA0_NVOOB_SQUELCH_FILTER_LENGTH_MASK        (0x3 << 26)
> +#define T_SATA0_NVOOB_SQUELCH_FILTER_LENGTH             (0x3 << 26)
> +
> +#define T_SATA_CFG_PHY_0                                0x120
> +#define T_SATA_CFG_PHY_0_USE_7BIT_ALIGN_DET_FOR_SPD     BIT(11)
> +#define T_SATA_CFG_PHY_0_MASK_SQUELCH                   BIT(24)
> +
> +#define FUSE_SATA_CALIB					0x124
> +#define FUSE_SATA_CALIB_MASK				0x3
> +
> +#define T_SATA0_CFG2NVOOB_2				0x134
> +#define T_SATA0_CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW_MASK	(0x1ff << 18)
> +#define T_SATA0_CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW	(0xc << 18)
>
>  #define T_SATA0_AHCI_HBA_CAP_BKDR			0x300
> +#define T_SATA0_AHCI_HBA_CAP_BKDR_PARTIAL_ST_CAP	BIT(13)
> +#define T_SATA0_AHCI_HBA_CAP_BKDR_SLUMBER_ST_CAP	BIT(14)
> +#define T_SATA0_AHCI_HBA_CAP_BKDR_SALP			BIT(26)
> +#define T_SATA0_AHCI_HBA_CAP_BKDR_SUPP_PM		BIT(17)
> +#define T_SATA0_AHCI_HBA_CAP_BKDR_SNCQ			BIT(30)
>
> -#define T_SATA0_BKDOOR_CC				0x4a4
> +#define T_SATA_BKDOOR_CC				0x4A4
> +#define T_SATA_BKDOOR_CC_CLASS_CODE_MASK		(0xFFFF << 16)
> +#define T_SATA_BKDOOR_CC_CLASS_CODE			(0x0106 << 16)
> +#define T_SATA_BKDOOR_CC_PROG_IF_MASK			(0xFF << 8)
> +#define T_SATA_BKDOOR_CC_PROG_IF			(0x01 << 8)
>
> -#define T_SATA0_CFG_SATA				0x54c
> -#define T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN		BIT(12)
> +#define T_SATA_CFG_SATA					0x54C
> +#define T_SATA_CFG_SATA_BACKDOOR_PROG_IF_EN		BIT(12)
>
>  #define T_SATA0_CFG_MISC				0x550
>
> @@ -82,8 +124,27 @@
>  #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
> +/* Electrical settings for better link stability */
> +#define T_SATA0_CHX_PHY_CTRL17_0			0x6e8
> +#define T_SATA0_CHX_PHY_CTRL17_0_RX_EQ_CTRL_L_GEN1	0x55010000
> +#define T_SATA0_CHX_PHY_CTRL18_0			0x6ec
> +#define T_SATA0_CHX_PHY_CTRL18_0_RX_EQ_CTRL_L_GEN2	0x55010000
> +#define T_SATA0_CHX_PHY_CTRL20_0			0x6f4
> +#define T_SATA0_CHX_PHY_CTRL20_0_RX_EQ_CTRL_H_GEN1	0x1
> +#define T_SATA0_CHX_PHY_CTRL21_0			0x6f8
> +#define T_SATA0_CHX_PHY_CTRL21_0_RX_EQ_CTRL_H_GEN2	0x1
> +
> +/* AUX Registers */
> +#define SATA_AUX_MISC_CNTL_1_0				0x8
> +#define DEVSLP_OVERRIDE					BIT(17)
> +#define SDS_SUPPORT					BIT(13)
> +#define DESO_SUPPORT					BIT(15)
> +
> +#define SATA_AUX_RX_STAT_INT_0				0xc
> +#define SATA_DEVSLP					BIT(7)
> +
> +#define SATA_AUX_SPARE_CFG0_0				0x18
> +#define MDAT_TIMER_AFTER_PG_VALID			BIT(14)
>
>  struct sata_pad_calibration {
>  	u8 gen1_tx_amp;
> @@ -99,15 +160,135 @@ static const struct sata_pad_calibration tegra124_pad_calibration[] = {
>  	{0x14, 0x0e, 0x1a, 0x0e},
>  };
>
> +struct tegra_ahci_ops {
> +	int (*init)(struct ahci_host_priv *);
> +};
> +
> +struct tegra_ahci_soc {
> +	const char *const *supply_names;
> +	unsigned int num_supplies;
> +	struct tegra_ahci_ops ops;
> +};
> +
>  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 platform_device *pdev;
> +	void __iomem *sata_regs;
> +	void __iomem *sata_aux_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];
> +	struct clk *sata_clk;
> +	struct regulator_bulk_data *supplies;
> +	struct tegra_ahci_soc *soc_data;
> +};

I don't particularly care whether the field names are aligned or not, 
but I remember the ata maintainers wanting them to be. Anyway, changing 
the style makes the patch larger and the actual functionality changes 
harder to review.

> +
> +static const char *const tegra124_supply_names[] = {
> +	"avdd", "hvdd", "vddio", "target-5v", "target-12v"
> +};
> +
> +static inline void tegra_ahci_sata_update(struct tegra_ahci_priv *tegra,
> +					  u32 val, u32 mask, u32 offset)
> +{
> +	u32 uval;
> +
> +	uval = readl(tegra->sata_regs + offset);
> +	uval = (uval & ~mask) | (val & mask);
> +	writel(uval, tegra->sata_regs + offset);
> +}

I'm not very fond of these _update functions. I think it is clearer to 
just write it out at the callsite, especially since this is used only 
four times.

> +
> +static inline void tegra_ahci_scfg_writel(struct tegra_ahci_priv *tegra,
> +					  u32 val, u32 offset)
> +{
> +	writel(val, tegra->sata_regs + SCFG_OFFSET + offset);
> +}
> +
> +static inline void tegra_ahci_scfg_update(struct tegra_ahci_priv *tegra,
> +					  u32 val, u32 mask, u32 offset)
> +{
> +	u32 uval;
> +
> +	uval = readl(tegra->sata_regs + SCFG_OFFSET + offset);
> +	uval = (uval & ~mask) | (val & mask);
> +	writel(uval, tegra->sata_regs + SCFG_OFFSET + offset);
> +}
> +
> +static inline u32 tegra_ahci_aux_readl(struct tegra_ahci_priv *tegra,
> +				       u32 offset)
> +{
> +	return readl(tegra->sata_aux_regs + offset);
> +}

This is never called.

> +
> +static inline void tegra_ahci_aux_update(struct tegra_ahci_priv *tegra, u32 val,
> +					 u32 mask, u32 offset)
> +{
> +	u32 uval;
> +
> +	uval = readl(tegra->sata_aux_regs + offset);
> +	uval = (uval & ~mask) | (val & mask);
> +	writel(uval, tegra->sata_aux_regs + offset);
> +}

This is only called once.

> +
> +static int tegra124_ahci_init(struct ahci_host_priv *hpriv)
> +{
> +	struct tegra_ahci_priv *tegra = hpriv->plat_data;
> +	struct sata_pad_calibration calib;
> +	int ret;
> +	u32 val;
> +	u32 mask;
> +
> +	/* Pad calibration */
> +	ret = tegra_fuse_readl(FUSE_SATA_CALIB, &val);
> +	if (ret)
> +		return ret;
> +
> +	calib = tegra124_pad_calibration[val & FUSE_SATA_CALIB_MASK];
> +
> +	tegra_ahci_scfg_writel(tegra, BIT(0), T_SATA0_INDEX);
> +
> +	mask = T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK |
> +	    T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK;
> +	val = (calib.gen1_tx_amp << T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT) |
> +	    (calib.gen1_tx_peak << T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT);
> +	tegra_ahci_scfg_update(tegra, val, mask, T_SATA0_CHX_PHY_CTRL1_GEN1);
> +
> +	mask = T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_MASK |
> +	    T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_MASK;
> +	val = (calib.gen2_tx_amp << T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT) |
> +	    (calib.gen2_tx_peak << T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT);
> +	tegra_ahci_scfg_update(tegra, val, mask, T_SATA0_CHX_PHY_CTRL1_GEN2);
> +
> +	tegra_ahci_scfg_writel(tegra,
> +			       T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ,
> +			       T_SATA0_CHX_PHY_CTRL11);
> +	tegra_ahci_scfg_writel(tegra,
> +			       T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1,
> +			       T_SATA0_CHX_PHY_CTRL2);
> +
> +	tegra_ahci_scfg_writel(tegra, 0, T_SATA0_INDEX);
> +
> +	return 0;
> +}
> +
> +static const struct tegra_ahci_soc tegra124_ahci_soc_data = {
> +	.supply_names = tegra124_supply_names,
> +	.num_supplies = ARRAY_SIZE(tegra124_supply_names),
> +	.ops = {
> +		.init = tegra124_ahci_init,
> +		},
> +};
> +
> +static const char *const tegra210_supply_names[] = {
> +	"dvdd-sata-pll",
> +	"hvdd-sata",
> +	"l0-hvddio-sata",
> +	"l0-dvddio-sata",
> +	"hvdd-pex-pll-e"
> +};

Perhaps the "-sata-" should be removed from these - I believe the supply 
name here should refer to the usage of the supplied power within the IP 
block. The IP block here is SATA so it is clear that it is used for 
something SATA related. If someone is better informed, please comment.

It also looks like this doesn't include the 5V and 12V supplies which 
had to be enabled on the Jetson TK1 to enable power output through the 
Molex connector - do you know if the Jetson TX1 no longer needs similar 
to enable the SATA power connector?

> +
> +static const struct tegra_ahci_soc tegra210_ahci_soc_data = {
> +	.supply_names = tegra210_supply_names,
> +	.num_supplies = ARRAY_SIZE(tegra210_supply_names),
>  };
>
>  static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
> @@ -115,7 +296,7 @@ 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),
> +	ret = regulator_bulk_enable(tegra->soc_data->num_supplies,
>  				    tegra->supplies);
>  	if (ret)
>  		return ret;
> @@ -144,8 +325,7 @@ static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
>  	tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
>
>  disable_regulators:
> -	regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra->supplies);
> -
> +	regulator_bulk_disable(tegra->soc_data->num_supplies, tegra->supplies);
>  	return ret;
>  }
>
> @@ -162,101 +342,124 @@ static void tegra_ahci_power_off(struct ahci_host_priv *hpriv)
>  	clk_disable_unprepare(tegra->sata_clk);
>  	tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
>
> -	regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra->supplies);
> +	regulator_bulk_disable(tegra->soc_data->num_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;
> +	u32 val;
> +	u32 mask;
>
>  	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 */
> -
> -	ret = tegra_fuse_readl(FUSE_SATA_CALIB, &val);
> -	if (ret) {
> -		dev_err(&tegra->pdev->dev,
> -			"failed to read calibration fuse: %d\n", ret);
> +	if (ret)
>  		return ret;
> -	}
> -
> -	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);
> +	/*
> +	 * Program the following  SATA IPFS registers
> +	 * to allow SW accesses to SATA's MMIO Register
> +	 */
> +	mask = FPCI_BAR5_START_MASK | FPCI_BAR5_ACCESS_TYPE;
> +	val = FPCI_BAR5_START | FPCI_BAR5_ACCESS_TYPE;
> +	tegra_ahci_sata_update(tegra, val, mask, SATA_FPCI_BAR5_0);
> +
> +	/* Program the following SATA IPFS register to enable the SATA */
> +	val = SATA_CONFIGURATION_0_EN_FPCI;
> +	tegra_ahci_sata_update(tegra, val, val, SATA_CONFIGURATION_0);
> +
> +	/* Electrical settings for better link stability */
> +	tegra_ahci_scfg_writel(tegra,
> +			       T_SATA0_CHX_PHY_CTRL17_0_RX_EQ_CTRL_L_GEN1,
> +			       T_SATA0_CHX_PHY_CTRL17_0);
> +	tegra_ahci_scfg_writel(tegra,
> +			       T_SATA0_CHX_PHY_CTRL18_0_RX_EQ_CTRL_L_GEN2,
> +			       T_SATA0_CHX_PHY_CTRL18_0);
> +	tegra_ahci_scfg_writel(tegra,
> +			       T_SATA0_CHX_PHY_CTRL20_0_RX_EQ_CTRL_H_GEN1,
> +			       T_SATA0_CHX_PHY_CTRL20_0);
> +	tegra_ahci_scfg_writel(tegra,
> +			       T_SATA0_CHX_PHY_CTRL21_0_RX_EQ_CTRL_H_GEN2,
> +			       T_SATA0_CHX_PHY_CTRL21_0);
> +
> +	/* For SQUELCH Filter & Gen3 drive getting detected as Gen1 drive */
> +
> +	mask = T_SATA_CFG_PHY_0_MASK_SQUELCH |
> +	    T_SATA_CFG_PHY_0_USE_7BIT_ALIGN_DET_FOR_SPD;
> +	val = T_SATA_CFG_PHY_0_MASK_SQUELCH;
> +	val &= ~T_SATA_CFG_PHY_0_USE_7BIT_ALIGN_DET_FOR_SPD;
> +	tegra_ahci_scfg_update(tegra, val, mask, T_SATA_CFG_PHY_0);
> +
> +	mask = (T_SATA0_NVOOB_COMMA_CNT_MASK |
> +		T_SATA0_NVOOB_SQUELCH_FILTER_LENGTH_MASK |
> +		T_SATA0_NVOOB_SQUELCH_FILTER_MODE_MASK);
> +	val = (T_SATA0_NVOOB_COMMA_CNT |
> +	       T_SATA0_NVOOB_SQUELCH_FILTER_LENGTH |
> +	       T_SATA0_NVOOB_SQUELCH_FILTER_MODE);
> +	tegra_ahci_scfg_update(tegra, val, mask, T_SATA0_NVOOB);
> +
> +	/*
> +	 * Change CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW from 83.3 ns to 58.8ns
> +	 */
> +	mask = T_SATA0_CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW_MASK;
> +	val = T_SATA0_CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW;
> +	tegra_ahci_scfg_update(tegra, val, mask, T_SATA0_CFG2NVOOB_2);
> +
> +	if (tegra->soc_data->ops.init)
> +		tegra->soc_data->ops.init(hpriv);
> +
> +	/*
> +	 * Program the following SATA configuration registers
> +	 * to initialize SATA
> +	 */
> +	val = (T_SATA_CFG_1_IO_SPACE | T_SATA_CFG_1_MEMORY_SPACE |
> +	       T_SATA_CFG_1_BUS_MASTER | T_SATA_CFG_1_SERR);
> +	tegra_ahci_scfg_update(tegra, val, val, T_SATA_CFG_1);
> +	tegra_ahci_scfg_writel(tegra, T_SATA_CFG_9_BASE_ADDRESS, T_SATA_CFG_9);
> +
> +	/* Program Class Code and Programming interface for SATA */
> +	val = T_SATA_CFG_SATA_BACKDOOR_PROG_IF_EN;
> +	tegra_ahci_scfg_update(tegra, val, val, T_SATA_CFG_SATA);
> +
> +	mask = T_SATA_BKDOOR_CC_CLASS_CODE_MASK | T_SATA_BKDOOR_CC_PROG_IF_MASK;
> +	val = T_SATA_BKDOOR_CC_CLASS_CODE | T_SATA_BKDOOR_CC_PROG_IF;
> +	tegra_ahci_scfg_update(tegra, val, mask, T_SATA_BKDOOR_CC);
> +
> +	tegra_ahci_scfg_update(tegra, 0, T_SATA_CFG_SATA_BACKDOOR_PROG_IF_EN,
> +			       T_SATA_CFG_SATA);
> +
> +	/* Enabling LPM capabilities through Backdoor Programming */
> +	val = (T_SATA0_AHCI_HBA_CAP_BKDR_PARTIAL_ST_CAP |
> +	       T_SATA0_AHCI_HBA_CAP_BKDR_SLUMBER_ST_CAP |
> +	       T_SATA0_AHCI_HBA_CAP_BKDR_SALP |
> +	       T_SATA0_AHCI_HBA_CAP_BKDR_SUPP_PM);
> +	tegra_ahci_scfg_update(tegra, val, val, T_SATA0_AHCI_HBA_CAP_BKDR);
> +
> +	/* SATA Second Level Clock Gating configuration
> +	 * Enabling Gating of Tx/Rx clocks and driving Pad IDDQ and Lane
> +	 * IDDQ Signals
> +	 */
> +	mask = T_SATA0_CFG_35_IDP_INDEX_MASK;
> +	val = T_SATA0_CFG_35_IDP_INDEX;
> +	tegra_ahci_scfg_update(tegra, val, mask, T_SATA0_CFG_35);
> +	tegra_ahci_scfg_writel(tegra, T_SATA0_AHCI_IDP1_DATA,
> +			       T_SATA0_AHCI_IDP1);
> +	val = (T_SATA0_CFG_PHY_1_PADS_IDDQ_EN |
> +	       T_SATA0_CFG_PHY_1_PAD_PLL_IDDQ_EN);
> +	tegra_ahci_scfg_update(tegra, val, val, T_SATA0_CFG_PHY_1);
> +
> +	/*
> +	 *  Indicate Sata only has the capability to enter DevSleep
> +	 * from slumber link.
> +	 */
> +	tegra_ahci_aux_update(tegra, DESO_SUPPORT, DESO_SUPPORT,
> +			      SATA_AUX_MISC_CNTL_1_0);
> +	/* Enabling IPFS Clock Gating */
> +	tegra_ahci_sata_update(tegra, 0, SATA_CONFIGURATION_CLK_OVERRIDE,
> +			       SATA_CONFIGURATION_0);
> +
> +	tegra_ahci_sata_update(tegra, IP_INT_MASK, IP_INT_MASK,
> +			       SATA_INTR_MASK_0);
>
>  	return 0;
>  }
> @@ -274,19 +477,24 @@ static void tegra_ahci_host_stop(struct ata_host *host)
>  }
>
>  static struct ata_port_operations ahci_tegra_port_ops = {
> -	.inherits	= &ahci_ops,
> -	.host_stop	= tegra_ahci_host_stop,
> +	.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,
> +	.flags = AHCI_FLAG_COMMON,
> +	.pio_mask = ATA_PIO4,
> +	.udma_mask = ATA_UDMA6,
> +	.port_ops = &ahci_tegra_port_ops,
>  };

Please keep the original style as to keep the patch as small as possible.

>
>  static const struct of_device_id tegra_ahci_of_match[] = {
> -	{ .compatible = "nvidia,tegra124-ahci" },
> +	{
> +	 .compatible = "nvidia,tegra124-ahci",
> +	 .data = &tegra124_ahci_soc_data},
> +	{
> +	 .compatible = "nvidia,tegra210-ahci",
> +	 .data = &tegra210_ahci_soc_data},

Please write these like

{
	.compatible = "nvidia,tegra124-ahci",
	.data = &tegra124_ahci_soc_data,
},

>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, tegra_ahci_of_match);
> @@ -301,6 +509,7 @@ static int tegra_ahci_probe(struct platform_device *pdev)
>  	struct tegra_ahci_priv *tegra;
>  	struct resource *res;
>  	int ret;
> +	unsigned int i;
>
>  	hpriv = ahci_platform_get_resources(pdev);
>  	if (IS_ERR(hpriv))
> @@ -311,13 +520,18 @@ static int tegra_ahci_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>
>  	hpriv->plat_data = tegra;
> -
>  	tegra->pdev = pdev;
> +	tegra->soc_data =
> +	    (struct tegra_ahci_soc *)of_device_get_match_data(&pdev->dev);
>
>  	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);
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> +	tegra->sata_aux_regs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(tegra->sata_aux_regs))
> +		return PTR_ERR(tegra->sata_aux_regs);

Please add this register range also to the device tree binding 
documentation. Also, please provide a patch to enable the SATA 
controller on a Tegra210 board - preferably the Jetson TX1 (or internal 
variant) so that it is easy to test.

>
>  	tegra->sata_rst = devm_reset_control_get(&pdev->dev, "sata");
>  	if (IS_ERR(tegra->sata_rst)) {
> @@ -343,13 +557,17 @@ static int tegra_ahci_probe(struct platform_device *pdev)
>  		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";
> +	tegra->supplies = devm_kcalloc(&pdev->dev,
> +				       tegra->soc_data->num_supplies,
> +				       sizeof(*tegra->supplies), GFP_KERNEL);
> +	if (!tegra->supplies)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < tegra->soc_data->num_supplies; i++)
> +		tegra->supplies[i].supply = tegra->soc_data->supply_names[i];
>
> -	ret = devm_regulator_bulk_get(&pdev->dev, ARRAY_SIZE(tegra->supplies),
> +	ret = devm_regulator_bulk_get(&pdev->dev,
> +				      tegra->soc_data->num_supplies,
>  				      tegra->supplies);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to get regulators\n");
> @@ -377,13 +595,13 @@ static struct platform_driver tegra_ahci_driver = {
>  	.probe = tegra_ahci_probe,
>  	.remove = ata_platform_remove_one,
>  	.driver = {
> -		.name = DRV_NAME,
> -		.of_match_table = tegra_ahci_of_match,
> -	},
> +		   .name = DRV_NAME,
> +		   .of_match_table = tegra_ahci_of_match,
> +		   },

Please keep the style unchanged here as well.

>  	/* LP0 suspend support not implemented */
>  };
>  module_platform_driver(tegra_ahci_driver);
>
>  MODULE_AUTHOR("Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>");
> -MODULE_DESCRIPTION("Tegra124 AHCI SATA driver");
> +MODULE_DESCRIPTION("Tegra AHCI SATA driver");

Awesome!

>  MODULE_LICENSE("GPL v2");
>
>

Thanks,
Mikko

RE: [v2,1/3] ata: ahci_tegra: add support for tegra210

[Preetham Chandru]

>-----Original Message-----
>From: Mikko Perttunen
>Sent: Monday, November 28, 2016 6:02 PM
>To: Preetham Chandru; preetham260@gmail.com
>Cc: tj@kernel.org; swarren@wwwdotorg.org; thierry.reding@gmail.com;
>Laxman Dewangan; linux-ide@vger.kernel.org; Venu Byravarasu; Pavan
>Kunapuli; linux-tegra@vger.kernel.org
>Subject: Re: [v2,1/3] ata: ahci_tegra: add support for tegra210
>
>+Cc linux-tegra
>
>Hi Preetham,
>
>I'll do a review pass. Please also Cc the next version to linux-
>tegra@vger.kernel.org so that more Tegra developers have visibility.
>

Ok.

>On 24.11.2016 09:43, PREETHAM RAMACHANDRA wrote:
>> From: Preetham Chandru R <pchandru@nvidia.com>
>>
>> Add AHCI support for tegra210
>> 1. Moved tegra124 specifics to tegra124_ahci_init.
>> 2. Separated out the regulators needed for tegra124 and tegra210.
>> 3. Set the LPM capabilities
>> 4. Create inline functions for read/write and modify to
>>    SATA, SATA Config and SATA Aux registers.
>>
>> Signed-off-by: Preetham Chandru R <pchandru@nvidia.com>
>> ---
>> v2:
>> * Fixed indentation issues
>> * Moved the change to disable DIPM, HIPM, DevSlp, partial,
>>   slumber and NCQ into a separate patch
>>
>>  drivers/ata/ahci_tegra.c | 478
>> ++++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 348 insertions(+), 130 deletions(-)
>>
>> diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c index
>> 3a62eb2..d12e2a9 100644
>> --- a/drivers/ata/ahci_tegra.c
>> +++ b/drivers/ata/ahci_tegra.c
>> @@ -33,32 +33,74 @@
>>
>>  #define DRV_NAME "tegra-ahci"
>>
>> +#define SATA_FPCI_BAR5_0				0x94
>> +#define FPCI_BAR5_START_MASK				(0xFFFFFFF <<
>4)
>> +#define FPCI_BAR5_START					(0x0040020
><< 4)
>> +#define FPCI_BAR5_ACCESS_TYPE				(0x1)
>
>Please use the existing convention in this file, that is, fields are prefixed with
>the register name. Also, please use small letters for hex digits as used in the
>file elsewhere.
>

Ok.

>> +
>>  #define SATA_CONFIGURATION_0				0x180
>> -#define SATA_CONFIGURATION_EN_FPCI			BIT(0)
>> +#define SATA_CONFIGURATION_0_EN_FPCI			BIT(0)
>> +#define SATA_CONFIGURATION_CLK_OVERRIDE			BIT(31)
>> +
>> +#define SATA_INTR_MASK_0				0x188
>> +#define IP_INT_MASK					BIT(16)
>>
>>  #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_SATA_CFG_1					0x4
>> +#define T_SATA_CFG_1_IO_SPACE				BIT(0)
>> +#define T_SATA_CFG_1_MEMORY_SPACE			BIT(1)
>> +#define T_SATA_CFG_1_BUS_MASTER				BIT(2)
>> +#define T_SATA_CFG_1_SERR				BIT(8)
>
>Try not to rename and move fields unnecessarily. It makes it very difficult to
>see where things have actually changed and makes the patch unnecessarily
>long.
>

Ok.

>> +
>> +#define T_SATA_CFG_9					0x24
>> +#define T_SATA_CFG_9_BASE_ADDRESS			0x40020000
>> +
>> +#define T_SATA0_CFG_35					0x94
>> +#define T_SATA0_CFG_35_IDP_INDEX_MASK			(0x7FF
><< 2)
>> +#define T_SATA0_CFG_35_IDP_INDEX			(0x2A << 2)
>>
>> -#define T_SATA0_CFG_9					0x24
>> -#define T_SATA0_CFG_9_BASE_ADDRESS_SHIFT		13
>> +#define T_SATA0_AHCI_IDP1				0x98
>> +#define T_SATA0_AHCI_IDP1_DATA
>	(0x400040)
>>
>> -#define SATA_FPCI_BAR5					0x94
>> -#define SATA_FPCI_BAR5_START_SHIFT			4
>> +#define T_SATA0_CFG_PHY_1				0x12C
>> +#define T_SATA0_CFG_PHY_1_PADS_IDDQ_EN			BIT(23)
>> +#define T_SATA0_CFG_PHY_1_PAD_PLL_IDDQ_EN		BIT(22)
>>
>> -#define SATA_INTR_MASK					0x188
>> -#define SATA_INTR_MASK_IP_INT_MASK			BIT(16)
>> +#define T_SATA0_NVOOB                                   0x114
>> +#define T_SATA0_NVOOB_COMMA_CNT_MASK                    (0xff << 16)
>> +#define T_SATA0_NVOOB_COMMA_CNT                         (0x07 << 16)
>> +#define T_SATA0_NVOOB_SQUELCH_FILTER_MODE_MASK          (0x3 <<
>24)
>> +#define T_SATA0_NVOOB_SQUELCH_FILTER_MODE               (0x1 << 24)
>> +#define T_SATA0_NVOOB_SQUELCH_FILTER_LENGTH_MASK        (0x3 <<
>26)
>> +#define T_SATA0_NVOOB_SQUELCH_FILTER_LENGTH             (0x3 << 26)
>> +
>> +#define T_SATA_CFG_PHY_0                                0x120
>> +#define T_SATA_CFG_PHY_0_USE_7BIT_ALIGN_DET_FOR_SPD     BIT(11)
>> +#define T_SATA_CFG_PHY_0_MASK_SQUELCH                   BIT(24)
>> +
>> +#define FUSE_SATA_CALIB					0x124
>> +#define FUSE_SATA_CALIB_MASK				0x3
>> +
>> +#define T_SATA0_CFG2NVOOB_2				0x134
>> +#define T_SATA0_CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW_MASK
>	(0x1ff << 18)
>> +#define T_SATA0_CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW	(0xc <<
>18)
>>
>>  #define T_SATA0_AHCI_HBA_CAP_BKDR			0x300
>> +#define T_SATA0_AHCI_HBA_CAP_BKDR_PARTIAL_ST_CAP	BIT(13)
>> +#define T_SATA0_AHCI_HBA_CAP_BKDR_SLUMBER_ST_CAP	BIT(14)
>> +#define T_SATA0_AHCI_HBA_CAP_BKDR_SALP			BIT(26)
>> +#define T_SATA0_AHCI_HBA_CAP_BKDR_SUPP_PM		BIT(17)
>> +#define T_SATA0_AHCI_HBA_CAP_BKDR_SNCQ			BIT(30)
>>
>> -#define T_SATA0_BKDOOR_CC				0x4a4
>> +#define T_SATA_BKDOOR_CC				0x4A4
>> +#define T_SATA_BKDOOR_CC_CLASS_CODE_MASK		(0xFFFF << 16)
>> +#define T_SATA_BKDOOR_CC_CLASS_CODE
>	(0x0106 << 16)
>> +#define T_SATA_BKDOOR_CC_PROG_IF_MASK			(0xFF
><< 8)
>> +#define T_SATA_BKDOOR_CC_PROG_IF			(0x01 << 8)
>>
>> -#define T_SATA0_CFG_SATA				0x54c
>> -#define T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN		BIT(12)
>> +#define T_SATA_CFG_SATA					0x54C
>> +#define T_SATA_CFG_SATA_BACKDOOR_PROG_IF_EN		BIT(12)
>>
>>  #define T_SATA0_CFG_MISC				0x550
>>
>> @@ -82,8 +124,27 @@
>>  #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
>> +/* Electrical settings for better link stability */
>> +#define T_SATA0_CHX_PHY_CTRL17_0			0x6e8
>> +#define T_SATA0_CHX_PHY_CTRL17_0_RX_EQ_CTRL_L_GEN1
>	0x55010000
>> +#define T_SATA0_CHX_PHY_CTRL18_0			0x6ec
>> +#define T_SATA0_CHX_PHY_CTRL18_0_RX_EQ_CTRL_L_GEN2
>	0x55010000
>> +#define T_SATA0_CHX_PHY_CTRL20_0			0x6f4
>> +#define T_SATA0_CHX_PHY_CTRL20_0_RX_EQ_CTRL_H_GEN1	0x1
>> +#define T_SATA0_CHX_PHY_CTRL21_0			0x6f8
>> +#define T_SATA0_CHX_PHY_CTRL21_0_RX_EQ_CTRL_H_GEN2	0x1
>> +
>> +/* AUX Registers */
>> +#define SATA_AUX_MISC_CNTL_1_0				0x8
>> +#define DEVSLP_OVERRIDE					BIT(17)
>> +#define SDS_SUPPORT					BIT(13)
>> +#define DESO_SUPPORT					BIT(15)
>> +
>> +#define SATA_AUX_RX_STAT_INT_0				0xc
>> +#define SATA_DEVSLP					BIT(7)
>> +
>> +#define SATA_AUX_SPARE_CFG0_0				0x18
>> +#define MDAT_TIMER_AFTER_PG_VALID			BIT(14)
>>
>>  struct sata_pad_calibration {
>>  	u8 gen1_tx_amp;
>> @@ -99,15 +160,135 @@ static const struct sata_pad_calibration
>tegra124_pad_calibration[] = {
>>  	{0x14, 0x0e, 0x1a, 0x0e},
>>  };
>>
>> +struct tegra_ahci_ops {
>> +	int (*init)(struct ahci_host_priv *); };
>> +
>> +struct tegra_ahci_soc {
>> +	const char *const *supply_names;
>> +	unsigned int num_supplies;
>> +	struct tegra_ahci_ops ops;
>> +};
>> +
>>  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 platform_device *pdev;
>> +	void __iomem *sata_regs;
>> +	void __iomem *sata_aux_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];
>> +	struct clk *sata_clk;
>> +	struct regulator_bulk_data *supplies;
>> +	struct tegra_ahci_soc *soc_data;
>> +};
>
>I don't particularly care whether the field names are aligned or not, but I
>remember the ata maintainers wanting them to be. Anyway, changing the
>style makes the patch larger and the actual functionality changes harder to
>review.
>

Ok will keep them aligned.

>> +
>> +static const char *const tegra124_supply_names[] = {
>> +	"avdd", "hvdd", "vddio", "target-5v", "target-12v"
>> +};
>> +
>> +static inline void tegra_ahci_sata_update(struct tegra_ahci_priv *tegra,
>> +					  u32 val, u32 mask, u32 offset) {
>> +	u32 uval;
>> +
>> +	uval = readl(tegra->sata_regs + offset);
>> +	uval = (uval & ~mask) | (val & mask);
>> +	writel(uval, tegra->sata_regs + offset); }
>
>I'm not very fond of these _update functions. I think it is clearer to just write
>it out at the callsite, especially since this is used only four times.
>

Ok will remove them.

>> +
>> +static inline void tegra_ahci_scfg_writel(struct tegra_ahci_priv *tegra,
>> +					  u32 val, u32 offset)
>> +{
>> +	writel(val, tegra->sata_regs + SCFG_OFFSET + offset); }
>> +
>> +static inline void tegra_ahci_scfg_update(struct tegra_ahci_priv *tegra,
>> +					  u32 val, u32 mask, u32 offset) {
>> +	u32 uval;
>> +
>> +	uval = readl(tegra->sata_regs + SCFG_OFFSET + offset);
>> +	uval = (uval & ~mask) | (val & mask);
>> +	writel(uval, tegra->sata_regs + SCFG_OFFSET + offset); }
>> +
>> +static inline u32 tegra_ahci_aux_readl(struct tegra_ahci_priv *tegra,
>> +				       u32 offset)
>> +{
>> +	return readl(tegra->sata_aux_regs + offset); }
>
>This is never called.
>

Will remove it.

>> +
>> +static inline void tegra_ahci_aux_update(struct tegra_ahci_priv *tegra,
>u32 val,
>> +					 u32 mask, u32 offset)
>> +{
>> +	u32 uval;
>> +
>> +	uval = readl(tegra->sata_aux_regs + offset);
>> +	uval = (uval & ~mask) | (val & mask);
>> +	writel(uval, tegra->sata_aux_regs + offset); }
>
>This is only called once.
>

Will remove it.


>> +
>> +static int tegra124_ahci_init(struct ahci_host_priv *hpriv) {
>> +	struct tegra_ahci_priv *tegra = hpriv->plat_data;
>> +	struct sata_pad_calibration calib;
>> +	int ret;
>> +	u32 val;
>> +	u32 mask;
>> +
>> +	/* Pad calibration */
>> +	ret = tegra_fuse_readl(FUSE_SATA_CALIB, &val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	calib = tegra124_pad_calibration[val & FUSE_SATA_CALIB_MASK];
>> +
>> +	tegra_ahci_scfg_writel(tegra, BIT(0), T_SATA0_INDEX);
>> +
>> +	mask = T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK |
>> +	    T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK;
>> +	val = (calib.gen1_tx_amp <<
>T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT) |
>> +	    (calib.gen1_tx_peak <<
>T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT);
>> +	tegra_ahci_scfg_update(tegra, val, mask,
>> +T_SATA0_CHX_PHY_CTRL1_GEN1);
>> +
>> +	mask = T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_MASK |
>> +	    T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_MASK;
>> +	val = (calib.gen2_tx_amp <<
>T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT) |
>> +	    (calib.gen2_tx_peak <<
>T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT);
>> +	tegra_ahci_scfg_update(tegra, val, mask,
>> +T_SATA0_CHX_PHY_CTRL1_GEN2);
>> +
>> +	tegra_ahci_scfg_writel(tegra,
>> +			       T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ,
>> +			       T_SATA0_CHX_PHY_CTRL11);
>> +	tegra_ahci_scfg_writel(tegra,
>> +			       T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1,
>> +			       T_SATA0_CHX_PHY_CTRL2);
>> +
>> +	tegra_ahci_scfg_writel(tegra, 0, T_SATA0_INDEX);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct tegra_ahci_soc tegra124_ahci_soc_data = {
>> +	.supply_names = tegra124_supply_names,
>> +	.num_supplies = ARRAY_SIZE(tegra124_supply_names),
>> +	.ops = {
>> +		.init = tegra124_ahci_init,
>> +		},
>> +};
>> +
>> +static const char *const tegra210_supply_names[] = {
>> +	"dvdd-sata-pll",
>> +	"hvdd-sata",
>> +	"l0-hvddio-sata",
>> +	"l0-dvddio-sata",
>> +	"hvdd-pex-pll-e"
>> +};
>
>Perhaps the "-sata-" should be removed from these - I believe the supply
>name here should refer to the usage of the supplied power within the IP
>block. The IP block here is SATA so it is clear that it is used for something
>SATA related. If someone is better informed, please comment.
>
>It also looks like this doesn't include the 5V and 12V supplies which had to
>be enabled on the Jetson TK1 to enable power output through the Molex
>connector - do you know if the Jetson TX1 no longer needs similar to enable
>the SATA power connector?
>

We need to keep regulator name matching with pin name. The regulatory framework and policy suggests that regulator name should match the pin name. So I have named it accordingly.  
The 5V and 12V does not seem to be required for tx1. I will take a closer look at it and if required I will push another patch for it.

>> +
>> +static const struct tegra_ahci_soc tegra210_ahci_soc_data = {
>> +	.supply_names = tegra210_supply_names,
>> +	.num_supplies = ARRAY_SIZE(tegra210_supply_names),
>>  };
>>
>>  static int tegra_ahci_power_on(struct ahci_host_priv *hpriv) @@
>> -115,7 +296,7 @@ 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),
>> +	ret = regulator_bulk_enable(tegra->soc_data->num_supplies,
>>  				    tegra->supplies);
>>  	if (ret)
>>  		return ret;
>> @@ -144,8 +325,7 @@ static int tegra_ahci_power_on(struct
>ahci_host_priv *hpriv)
>>  	tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
>>
>>  disable_regulators:
>> -	regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra-
>>supplies);
>> -
>> +	regulator_bulk_disable(tegra->soc_data->num_supplies,
>> +tegra->supplies);
>>  	return ret;
>>  }
>>
>> @@ -162,101 +342,124 @@ static void tegra_ahci_power_off(struct
>ahci_host_priv *hpriv)
>>  	clk_disable_unprepare(tegra->sata_clk);
>>  	tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
>>
>> -	regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra-
>>supplies);
>> +	regulator_bulk_disable(tegra->soc_data->num_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;
>> +	u32 val;
>> +	u32 mask;
>>
>>  	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 */
>> -
>> -	ret = tegra_fuse_readl(FUSE_SATA_CALIB, &val);
>> -	if (ret) {
>> -		dev_err(&tegra->pdev->dev,
>> -			"failed to read calibration fuse: %d\n", ret);
>> +	if (ret)
>>  		return ret;
>> -	}
>> -
>> -	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);
>> +	/*
>> +	 * Program the following  SATA IPFS registers
>> +	 * to allow SW accesses to SATA's MMIO Register
>> +	 */
>> +	mask = FPCI_BAR5_START_MASK | FPCI_BAR5_ACCESS_TYPE;
>> +	val = FPCI_BAR5_START | FPCI_BAR5_ACCESS_TYPE;
>> +	tegra_ahci_sata_update(tegra, val, mask, SATA_FPCI_BAR5_0);
>> +
>> +	/* Program the following SATA IPFS register to enable the SATA */
>> +	val = SATA_CONFIGURATION_0_EN_FPCI;
>> +	tegra_ahci_sata_update(tegra, val, val, SATA_CONFIGURATION_0);
>> +
>> +	/* Electrical settings for better link stability */
>> +	tegra_ahci_scfg_writel(tegra,
>> +
>T_SATA0_CHX_PHY_CTRL17_0_RX_EQ_CTRL_L_GEN1,
>> +			       T_SATA0_CHX_PHY_CTRL17_0);
>> +	tegra_ahci_scfg_writel(tegra,
>> +
>T_SATA0_CHX_PHY_CTRL18_0_RX_EQ_CTRL_L_GEN2,
>> +			       T_SATA0_CHX_PHY_CTRL18_0);
>> +	tegra_ahci_scfg_writel(tegra,
>> +
>T_SATA0_CHX_PHY_CTRL20_0_RX_EQ_CTRL_H_GEN1,
>> +			       T_SATA0_CHX_PHY_CTRL20_0);
>> +	tegra_ahci_scfg_writel(tegra,
>> +
>T_SATA0_CHX_PHY_CTRL21_0_RX_EQ_CTRL_H_GEN2,
>> +			       T_SATA0_CHX_PHY_CTRL21_0);
>> +
>> +	/* For SQUELCH Filter & Gen3 drive getting detected as Gen1 drive
>*/
>> +
>> +	mask = T_SATA_CFG_PHY_0_MASK_SQUELCH |
>> +	    T_SATA_CFG_PHY_0_USE_7BIT_ALIGN_DET_FOR_SPD;
>> +	val = T_SATA_CFG_PHY_0_MASK_SQUELCH;
>> +	val &= ~T_SATA_CFG_PHY_0_USE_7BIT_ALIGN_DET_FOR_SPD;
>> +	tegra_ahci_scfg_update(tegra, val, mask, T_SATA_CFG_PHY_0);
>> +
>> +	mask = (T_SATA0_NVOOB_COMMA_CNT_MASK |
>> +		T_SATA0_NVOOB_SQUELCH_FILTER_LENGTH_MASK |
>> +		T_SATA0_NVOOB_SQUELCH_FILTER_MODE_MASK);
>> +	val = (T_SATA0_NVOOB_COMMA_CNT |
>> +	       T_SATA0_NVOOB_SQUELCH_FILTER_LENGTH |
>> +	       T_SATA0_NVOOB_SQUELCH_FILTER_MODE);
>> +	tegra_ahci_scfg_update(tegra, val, mask, T_SATA0_NVOOB);
>> +
>> +	/*
>> +	 * Change CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW from 83.3 ns
>to 58.8ns
>> +	 */
>> +	mask =
>T_SATA0_CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW_MASK;
>> +	val = T_SATA0_CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW;
>> +	tegra_ahci_scfg_update(tegra, val, mask, T_SATA0_CFG2NVOOB_2);
>> +
>> +	if (tegra->soc_data->ops.init)
>> +		tegra->soc_data->ops.init(hpriv);
>> +
>> +	/*
>> +	 * Program the following SATA configuration registers
>> +	 * to initialize SATA
>> +	 */
>> +	val = (T_SATA_CFG_1_IO_SPACE | T_SATA_CFG_1_MEMORY_SPACE
>|
>> +	       T_SATA_CFG_1_BUS_MASTER | T_SATA_CFG_1_SERR);
>> +	tegra_ahci_scfg_update(tegra, val, val, T_SATA_CFG_1);
>> +	tegra_ahci_scfg_writel(tegra, T_SATA_CFG_9_BASE_ADDRESS,
>> +T_SATA_CFG_9);
>> +
>> +	/* Program Class Code and Programming interface for SATA */
>> +	val = T_SATA_CFG_SATA_BACKDOOR_PROG_IF_EN;
>> +	tegra_ahci_scfg_update(tegra, val, val, T_SATA_CFG_SATA);
>> +
>> +	mask = T_SATA_BKDOOR_CC_CLASS_CODE_MASK |
>T_SATA_BKDOOR_CC_PROG_IF_MASK;
>> +	val = T_SATA_BKDOOR_CC_CLASS_CODE |
>T_SATA_BKDOOR_CC_PROG_IF;
>> +	tegra_ahci_scfg_update(tegra, val, mask, T_SATA_BKDOOR_CC);
>> +
>> +	tegra_ahci_scfg_update(tegra, 0,
>T_SATA_CFG_SATA_BACKDOOR_PROG_IF_EN,
>> +			       T_SATA_CFG_SATA);
>> +
>> +	/* Enabling LPM capabilities through Backdoor Programming */
>> +	val = (T_SATA0_AHCI_HBA_CAP_BKDR_PARTIAL_ST_CAP |
>> +	       T_SATA0_AHCI_HBA_CAP_BKDR_SLUMBER_ST_CAP |
>> +	       T_SATA0_AHCI_HBA_CAP_BKDR_SALP |
>> +	       T_SATA0_AHCI_HBA_CAP_BKDR_SUPP_PM);
>> +	tegra_ahci_scfg_update(tegra, val, val,
>T_SATA0_AHCI_HBA_CAP_BKDR);
>> +
>> +	/* SATA Second Level Clock Gating configuration
>> +	 * Enabling Gating of Tx/Rx clocks and driving Pad IDDQ and Lane
>> +	 * IDDQ Signals
>> +	 */
>> +	mask = T_SATA0_CFG_35_IDP_INDEX_MASK;
>> +	val = T_SATA0_CFG_35_IDP_INDEX;
>> +	tegra_ahci_scfg_update(tegra, val, mask, T_SATA0_CFG_35);
>> +	tegra_ahci_scfg_writel(tegra, T_SATA0_AHCI_IDP1_DATA,
>> +			       T_SATA0_AHCI_IDP1);
>> +	val = (T_SATA0_CFG_PHY_1_PADS_IDDQ_EN |
>> +	       T_SATA0_CFG_PHY_1_PAD_PLL_IDDQ_EN);
>> +	tegra_ahci_scfg_update(tegra, val, val, T_SATA0_CFG_PHY_1);
>> +
>> +	/*
>> +	 *  Indicate Sata only has the capability to enter DevSleep
>> +	 * from slumber link.
>> +	 */
>> +	tegra_ahci_aux_update(tegra, DESO_SUPPORT, DESO_SUPPORT,
>> +			      SATA_AUX_MISC_CNTL_1_0);
>> +	/* Enabling IPFS Clock Gating */
>> +	tegra_ahci_sata_update(tegra, 0,
>SATA_CONFIGURATION_CLK_OVERRIDE,
>> +			       SATA_CONFIGURATION_0);
>> +
>> +	tegra_ahci_sata_update(tegra, IP_INT_MASK, IP_INT_MASK,
>> +			       SATA_INTR_MASK_0);
>>
>>  	return 0;
>>  }
>> @@ -274,19 +477,24 @@ static void tegra_ahci_host_stop(struct
>ata_host
>> *host)  }
>>
>>  static struct ata_port_operations ahci_tegra_port_ops = {
>> -	.inherits	= &ahci_ops,
>> -	.host_stop	= tegra_ahci_host_stop,
>> +	.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,
>> +	.flags = AHCI_FLAG_COMMON,
>> +	.pio_mask = ATA_PIO4,
>> +	.udma_mask = ATA_UDMA6,
>> +	.port_ops = &ahci_tegra_port_ops,
>>  };
>
>Please keep the original style as to keep the patch as small as possible.
>

Ok.

>>
>>  static const struct of_device_id tegra_ahci_of_match[] = {
>> -	{ .compatible = "nvidia,tegra124-ahci" },
>> +	{
>> +	 .compatible = "nvidia,tegra124-ahci",
>> +	 .data = &tegra124_ahci_soc_data},
>> +	{
>> +	 .compatible = "nvidia,tegra210-ahci",
>> +	 .data = &tegra210_ahci_soc_data},
>
>Please write these like
>
>{
>	.compatible = "nvidia,tegra124-ahci",
>	.data = &tegra124_ahci_soc_data,
>},
>

Ok.

>>  	{}
>>  };
>>  MODULE_DEVICE_TABLE(of, tegra_ahci_of_match); @@ -301,6 +509,7
>@@
>> static int tegra_ahci_probe(struct platform_device *pdev)
>>  	struct tegra_ahci_priv *tegra;
>>  	struct resource *res;
>>  	int ret;
>> +	unsigned int i;
>>
>>  	hpriv = ahci_platform_get_resources(pdev);
>>  	if (IS_ERR(hpriv))
>> @@ -311,13 +520,18 @@ static int tegra_ahci_probe(struct
>platform_device *pdev)
>>  		return -ENOMEM;
>>
>>  	hpriv->plat_data = tegra;
>> -
>>  	tegra->pdev = pdev;
>> +	tegra->soc_data =
>> +	    (struct tegra_ahci_soc *)of_device_get_match_data(&pdev->dev);
>>
>>  	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);
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
>> +	tegra->sata_aux_regs = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(tegra->sata_aux_regs))
>> +		return PTR_ERR(tegra->sata_aux_regs);
>
>Please add this register range also to the device tree binding documentation.
>Also, please provide a patch to enable the SATA controller on a Tegra210
>board - preferably the Jetson TX1 (or internal
>variant) so that it is easy to test.
>

Ok. 

>>
>>  	tegra->sata_rst = devm_reset_control_get(&pdev->dev, "sata");
>>  	if (IS_ERR(tegra->sata_rst)) {
>> @@ -343,13 +557,17 @@ static int tegra_ahci_probe(struct
>platform_device *pdev)
>>  		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";
>> +	tegra->supplies = devm_kcalloc(&pdev->dev,
>> +				       tegra->soc_data->num_supplies,
>> +				       sizeof(*tegra->supplies), GFP_KERNEL);
>> +	if (!tegra->supplies)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < tegra->soc_data->num_supplies; i++)
>> +		tegra->supplies[i].supply = tegra->soc_data-
>>supply_names[i];
>>
>> -	ret = devm_regulator_bulk_get(&pdev->dev, ARRAY_SIZE(tegra-
>>supplies),
>> +	ret = devm_regulator_bulk_get(&pdev->dev,
>> +				      tegra->soc_data->num_supplies,
>>  				      tegra->supplies);
>>  	if (ret) {
>>  		dev_err(&pdev->dev, "Failed to get regulators\n"); @@ -
>377,13
>> +595,13 @@ static struct platform_driver tegra_ahci_driver = {
>>  	.probe = tegra_ahci_probe,
>>  	.remove = ata_platform_remove_one,
>>  	.driver = {
>> -		.name = DRV_NAME,
>> -		.of_match_table = tegra_ahci_of_match,
>> -	},
>> +		   .name = DRV_NAME,
>> +		   .of_match_table = tegra_ahci_of_match,
>> +		   },
>
>Please keep the style unchanged here as well.
>

Ok.


>>  	/* 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_DESCRIPTION("Tegra AHCI SATA driver");
>
>Awesome!
>
>>  MODULE_LICENSE("GPL v2");
>>
>>
>
>Thanks,
>Mikko

Re: [v2,1/3] ata: ahci_tegra: add support for tegra210

[Mikko Perttunen]

On 21.12.2016 10:50, Preetham Chandru wrote:
>...
>>
>> Perhaps the "-sata-" should be removed from these - I believe the supply
>> name here should refer to the usage of the supplied power within the IP
>> block. The IP block here is SATA so it is clear that it is used for something
>> SATA related. If someone is better informed, please comment.
>>
>> It also looks like this doesn't include the 5V and 12V supplies which had to
>> be enabled on the Jetson TK1 to enable power output through the Molex
>> connector - do you know if the Jetson TX1 no longer needs similar to enable
>> the SATA power connector?
>>
>
> We need to keep regulator name matching with pin name. The regulatory framework and policy suggests that regulator name should match the pin name. So I have named it accordingly.
> The 5V and 12V does not seem to be required for tx1. I will take a closer look at it and if required I will push another patch for it.

Ok, that makes sense; sounds good to me.

Thanks,
Mikko.

[PATCH v2 2/3] ata: ahci_tegra: Add support to disable features

[Preetham Chandru Ramchandra]

From: Preetham Chandru R <pchandru@nvidia.com>

Add support to disable DIPM, HIPM, DevSlp, partial, slumber and NCQ
features from DT. By default these features are enabled.

Signed-off-by: Preetham Chandru R <pchandru@nvidia.com>
---
v2:
* This change was created by seperating
   "ata: ahci_tegra: add support for tegra210" from v1

 drivers/ata/ahci_tegra.c | 107 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 82 insertions(+), 25 deletions(-)

diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c
index d12e2a9..443c3e8 100644
--- a/drivers/ata/ahci_tegra.c
+++ b/drivers/ata/ahci_tegra.c
@@ -329,7 +329,7 @@ static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
 	return ret;
 }
 
-static void tegra_ahci_power_off(struct ahci_host_priv *hpriv)
+static void tegra_ahci_controller_deinit(struct ahci_host_priv *hpriv)
 {
 	struct tegra_ahci_priv *tegra = hpriv->plat_data;
 
@@ -345,6 +345,85 @@ static void tegra_ahci_power_off(struct ahci_host_priv *hpriv)
 	regulator_bulk_disable(tegra->soc_data->num_supplies, tegra->supplies);
 }
 
+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 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 void tegra_ahci_disable_devslp(struct tegra_ahci_priv *tegra)
+{
+	tegra_ahci_aux_update(tegra, 0, SDS_SUPPORT, SATA_AUX_MISC_CNTL_1_0);
+}
+
+static void tegra_ahci_disable_hipm(struct tegra_ahci_priv *tegra)
+{
+	tegra_ahci_scfg_update(tegra, 0, T_SATA0_AHCI_HBA_CAP_BKDR_SALP,
+			       T_SATA0_AHCI_HBA_CAP_BKDR);
+}
+
+static void tegra_ahci_disable_partial(struct tegra_ahci_priv *tegra)
+{
+	tegra_ahci_scfg_update(tegra, 0,
+			       T_SATA0_AHCI_HBA_CAP_BKDR_PARTIAL_ST_CAP,
+			       T_SATA0_AHCI_HBA_CAP_BKDR);
+}
+
+static void tegra_ahci_disable_slumber(struct tegra_ahci_priv *tegra)
+{
+	tegra_ahci_scfg_update(tegra, 0,
+			       T_SATA0_AHCI_HBA_CAP_BKDR_SLUMBER_ST_CAP,
+			       T_SATA0_AHCI_HBA_CAP_BKDR);
+}
+
+static void tegra_ahci_disable_ncq(struct tegra_ahci_priv *tegra)
+{
+	tegra_ahci_scfg_update(tegra, 0, T_SATA0_AHCI_HBA_CAP_BKDR_SNCQ,
+			       T_SATA0_AHCI_HBA_CAP_BKDR);
+}
+
+static void tegra_ahci_disable_features(struct ahci_host_priv *hpriv)
+{
+	struct tegra_ahci_priv *tegra = hpriv->plat_data;
+	struct platform_device *pdev = tegra->pdev;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct property *prop;
+	const char *feature;
+
+	if (of_property_count_strings(np, "nvidia,disable-features") <= 0)
+		return;
+
+	of_property_for_each_string(np, "nvidia,disable-features", prop,
+				    feature) {
+		if (!strcmp(feature, "devslp"))
+			tegra_ahci_disable_devslp(tegra);
+		else if (!strcmp(feature, "hipm"))
+			tegra_ahci_disable_hipm(tegra);
+		else if (!strcmp(feature, "ncq"))
+			tegra_ahci_disable_ncq(tegra);
+		else if (!strcmp(feature, "dipm"))
+			ahci_tegra_port_info.flags |= ATA_FLAG_NO_DIPM;
+		else if (!strcmp(feature, "partial"))
+			tegra_ahci_disable_partial(tegra);
+		else if (!strcmp(feature, "slumber"))
+			tegra_ahci_disable_slumber(tegra);
+	}
+}
+
 static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv)
 {
 	struct tegra_ahci_priv *tegra = hpriv->plat_data;
@@ -458,36 +537,14 @@ static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv)
 	tegra_ahci_sata_update(tegra, 0, SATA_CONFIGURATION_CLK_OVERRIDE,
 			       SATA_CONFIGURATION_0);
 
+	tegra_ahci_disable_features(hpriv);
+
 	tegra_ahci_sata_update(tegra, IP_INT_MASK, IP_INT_MASK,
 			       SATA_INTR_MASK_0);
 
 	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",
-- 
2.1.4

Re: [v2,2/3] ata: ahci_tegra: Add support to disable features

[Mikko Perttunen]

On 24.11.2016 09:43, PREETHAM RAMACHANDRA wrote:
> From: Preetham Chandru R <pchandru-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> Add support to disable DIPM, HIPM, DevSlp, partial, slumber and NCQ
> features from DT. By default these features are enabled.
>

Why are these features disabled? Are they broken on all Tegra210 chips, 
broken on specific boards or do they require some support from the 
driver that is not there?

Most likely we should hardcode the disabled features into the driver 
instead of reading them from the device tree.

> Signed-off-by: Preetham Chandru R <pchandru-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> v2:
> * This change was created by seperating
>    "ata: ahci_tegra: add support for tegra210" from v1
>
>  drivers/ata/ahci_tegra.c | 107 ++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 82 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c
> index d12e2a9..443c3e8 100644
> --- a/drivers/ata/ahci_tegra.c
> +++ b/drivers/ata/ahci_tegra.c
> @@ -329,7 +329,7 @@ static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
>  	return ret;
>  }
>
> -static void tegra_ahci_power_off(struct ahci_host_priv *hpriv)
> +static void tegra_ahci_controller_deinit(struct ahci_host_priv *hpriv)
>  {
>  	struct tegra_ahci_priv *tegra = hpriv->plat_data;
>
> @@ -345,6 +345,85 @@ static void tegra_ahci_power_off(struct ahci_host_priv *hpriv)
>  	regulator_bulk_disable(tegra->soc_data->num_supplies, tegra->supplies);
>  }
>
> +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 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 void tegra_ahci_disable_devslp(struct tegra_ahci_priv *tegra)
> +{
> +	tegra_ahci_aux_update(tegra, 0, SDS_SUPPORT, SATA_AUX_MISC_CNTL_1_0);
> +}
> +
> +static void tegra_ahci_disable_hipm(struct tegra_ahci_priv *tegra)
> +{
> +	tegra_ahci_scfg_update(tegra, 0, T_SATA0_AHCI_HBA_CAP_BKDR_SALP,
> +			       T_SATA0_AHCI_HBA_CAP_BKDR);
> +}
> +
> +static void tegra_ahci_disable_partial(struct tegra_ahci_priv *tegra)
> +{
> +	tegra_ahci_scfg_update(tegra, 0,
> +			       T_SATA0_AHCI_HBA_CAP_BKDR_PARTIAL_ST_CAP,
> +			       T_SATA0_AHCI_HBA_CAP_BKDR);
> +}
> +
> +static void tegra_ahci_disable_slumber(struct tegra_ahci_priv *tegra)
> +{
> +	tegra_ahci_scfg_update(tegra, 0,
> +			       T_SATA0_AHCI_HBA_CAP_BKDR_SLUMBER_ST_CAP,
> +			       T_SATA0_AHCI_HBA_CAP_BKDR);
> +}
> +
> +static void tegra_ahci_disable_ncq(struct tegra_ahci_priv *tegra)
> +{
> +	tegra_ahci_scfg_update(tegra, 0, T_SATA0_AHCI_HBA_CAP_BKDR_SNCQ,
> +			       T_SATA0_AHCI_HBA_CAP_BKDR);
> +}

These probably don't need their own functions.

> +
> +static void tegra_ahci_disable_features(struct ahci_host_priv *hpriv)
> +{
> +	struct tegra_ahci_priv *tegra = hpriv->plat_data;
> +	struct platform_device *pdev = tegra->pdev;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct property *prop;
> +	const char *feature;
> +
> +	if (of_property_count_strings(np, "nvidia,disable-features") <= 0)
> +		return;
> +
> +	of_property_for_each_string(np, "nvidia,disable-features", prop,
> +				    feature) {
> +		if (!strcmp(feature, "devslp"))
> +			tegra_ahci_disable_devslp(tegra);
> +		else if (!strcmp(feature, "hipm"))
> +			tegra_ahci_disable_hipm(tegra);
> +		else if (!strcmp(feature, "ncq"))
> +			tegra_ahci_disable_ncq(tegra);
> +		else if (!strcmp(feature, "dipm"))
> +			ahci_tegra_port_info.flags |= ATA_FLAG_NO_DIPM;
> +		else if (!strcmp(feature, "partial"))
> +			tegra_ahci_disable_partial(tegra);
> +		else if (!strcmp(feature, "slumber"))
> +			tegra_ahci_disable_slumber(tegra);
> +	}
> +}
> +
>  static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv)
>  {
>  	struct tegra_ahci_priv *tegra = hpriv->plat_data;
> @@ -458,36 +537,14 @@ static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv)
>  	tegra_ahci_sata_update(tegra, 0, SATA_CONFIGURATION_CLK_OVERRIDE,
>  			       SATA_CONFIGURATION_0);
>
> +	tegra_ahci_disable_features(hpriv);
> +
>  	tegra_ahci_sata_update(tegra, IP_INT_MASK, IP_INT_MASK,
>  			       SATA_INTR_MASK_0);
>
>  	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",
>
>

Thanks,
Mikko.

RE: [v2,2/3] ata: ahci_tegra: Add support to disable features

[Preetham Chandru]

>-----Original Message-----
>From: Mikko Perttunen
>Sent: Monday, November 28, 2016 6:09 PM
>To: Preetham Chandru; tj@kernel.org; swarren@wwwdotorg.org;
>thierry.reding@gmail.com; preetham260@gmail.com
>Cc: Laxman Dewangan; linux-ide@vger.kernel.org; Venu Byravarasu; Pavan
>Kunapuli; linux-tegra@vger.kernel.org
>Subject: Re: [v2,2/3] ata: ahci_tegra: Add support to disable features
>
>On 24.11.2016 09:43, PREETHAM RAMACHANDRA wrote:
>> From: Preetham Chandru R <pchandru@nvidia.com>
>>
>> Add support to disable DIPM, HIPM, DevSlp, partial, slumber and NCQ
>> features from DT. By default these features are enabled.
>>
>
>Why are these features disabled? Are they broken on all Tegra210 chips,
>broken on specific boards or do they require some support from the driver
>that is not there?
>
>Most likely we should hardcode the disabled features into the driver instead
>of reading them from the device tree.
>

Yes, currently DevSlp and DIPM features are broken for t210 and t124 but devslp is fixed for tegra186. 
 Also I thought it would be nice to provide similar options for other features as well.
Since we do not support hotplug this would help us in debugging if we face issues with certain drives during kernel boot.
We can disable features we think are causing issues and retry.

>> Signed-off-by: Preetham Chandru R <pchandru@nvidia.com>
>> ---
>> v2:
>> * This change was created by seperating
>>    "ata: ahci_tegra: add support for tegra210" from v1
>>
>>  drivers/ata/ahci_tegra.c | 107
>> ++++++++++++++++++++++++++++++++++++-----------
>>  1 file changed, 82 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c index
>> d12e2a9..443c3e8 100644
>> --- a/drivers/ata/ahci_tegra.c
>> +++ b/drivers/ata/ahci_tegra.c
>> @@ -329,7 +329,7 @@ static int tegra_ahci_power_on(struct
>ahci_host_priv *hpriv)
>>  	return ret;
>>  }
>>
>> -static void tegra_ahci_power_off(struct ahci_host_priv *hpriv)
>> +static void tegra_ahci_controller_deinit(struct ahci_host_priv
>> +*hpriv)
>>  {
>>  	struct tegra_ahci_priv *tegra = hpriv->plat_data;
>>
>> @@ -345,6 +345,85 @@ static void tegra_ahci_power_off(struct
>ahci_host_priv *hpriv)
>>  	regulator_bulk_disable(tegra->soc_data->num_supplies,
>> tegra->supplies);  }
>>
>> +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 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 void tegra_ahci_disable_devslp(struct tegra_ahci_priv *tegra)
>> +{
>> +	tegra_ahci_aux_update(tegra, 0, SDS_SUPPORT,
>> +SATA_AUX_MISC_CNTL_1_0); }
>> +
>> +static void tegra_ahci_disable_hipm(struct tegra_ahci_priv *tegra) {
>> +	tegra_ahci_scfg_update(tegra, 0,
>T_SATA0_AHCI_HBA_CAP_BKDR_SALP,
>> +			       T_SATA0_AHCI_HBA_CAP_BKDR); }
>> +
>> +static void tegra_ahci_disable_partial(struct tegra_ahci_priv *tegra)
>> +{
>> +	tegra_ahci_scfg_update(tegra, 0,
>> +
>T_SATA0_AHCI_HBA_CAP_BKDR_PARTIAL_ST_CAP,
>> +			       T_SATA0_AHCI_HBA_CAP_BKDR); }
>> +
>> +static void tegra_ahci_disable_slumber(struct tegra_ahci_priv *tegra)
>> +{
>> +	tegra_ahci_scfg_update(tegra, 0,
>> +
>T_SATA0_AHCI_HBA_CAP_BKDR_SLUMBER_ST_CAP,
>> +			       T_SATA0_AHCI_HBA_CAP_BKDR); }
>> +
>> +static void tegra_ahci_disable_ncq(struct tegra_ahci_priv *tegra) {
>> +	tegra_ahci_scfg_update(tegra, 0,
>T_SATA0_AHCI_HBA_CAP_BKDR_SNCQ,
>> +			       T_SATA0_AHCI_HBA_CAP_BKDR); }
>
>These probably don't need their own functions.
>
Ok

>> +
>> +static void tegra_ahci_disable_features(struct ahci_host_priv *hpriv)
>> +{
>> +	struct tegra_ahci_priv *tegra = hpriv->plat_data;
>> +	struct platform_device *pdev = tegra->pdev;
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *np = dev->of_node;
>> +	struct property *prop;
>> +	const char *feature;
>> +
>> +	if (of_property_count_strings(np, "nvidia,disable-features") <= 0)
>> +		return;
>> +
>> +	of_property_for_each_string(np, "nvidia,disable-features", prop,
>> +				    feature) {
>> +		if (!strcmp(feature, "devslp"))
>> +			tegra_ahci_disable_devslp(tegra);
>> +		else if (!strcmp(feature, "hipm"))
>> +			tegra_ahci_disable_hipm(tegra);
>> +		else if (!strcmp(feature, "ncq"))
>> +			tegra_ahci_disable_ncq(tegra);
>> +		else if (!strcmp(feature, "dipm"))
>> +			ahci_tegra_port_info.flags |= ATA_FLAG_NO_DIPM;
>> +		else if (!strcmp(feature, "partial"))
>> +			tegra_ahci_disable_partial(tegra);
>> +		else if (!strcmp(feature, "slumber"))
>> +			tegra_ahci_disable_slumber(tegra);
>> +	}
>> +}
>> +
>>  static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv)
>> {
>>  	struct tegra_ahci_priv *tegra = hpriv->plat_data; @@ -458,36
>+537,14
>> @@ static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv)
>>  	tegra_ahci_sata_update(tegra, 0,
>SATA_CONFIGURATION_CLK_OVERRIDE,
>>  			       SATA_CONFIGURATION_0);
>>
>> +	tegra_ahci_disable_features(hpriv);
>> +
>>  	tegra_ahci_sata_update(tegra, IP_INT_MASK, IP_INT_MASK,
>>  			       SATA_INTR_MASK_0);
>>
>>  	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",
>>
>>
>
>Thanks,
>Mikko.

Thanks,
Preetham.

Re: [v2,2/3] ata: ahci_tegra: Add support to disable features

[Mikko Perttunen]

On 21.12.2016 11:12, Preetham Chandru wrote:
>
>
>> -----Original Message-----
>> From: Mikko Perttunen
>> Sent: Monday, November 28, 2016 6:09 PM
>> To: Preetham Chandru; tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org;
>> thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; preetham260-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
>> Cc: Laxman Dewangan; linux-ide-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Venu Byravarasu; Pavan
>> Kunapuli; linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Subject: Re: [v2,2/3] ata: ahci_tegra: Add support to disable features
>>
>> On 24.11.2016 09:43, PREETHAM RAMACHANDRA wrote:
>>> From: Preetham Chandru R <pchandru-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>
>>> Add support to disable DIPM, HIPM, DevSlp, partial, slumber and NCQ
>>> features from DT. By default these features are enabled.
>>>
>>
>> Why are these features disabled? Are they broken on all Tegra210 chips,
>> broken on specific boards or do they require some support from the driver
>> that is not there?
>>
>> Most likely we should hardcode the disabled features into the driver instead
>> of reading them from the device tree.
>>
>
> Yes, currently DevSlp and DIPM features are broken for t210 and t124 but devslp is fixed for tegra186.
>  Also I thought it would be nice to provide similar options for other features as well.
> Since we do not support hotplug this would help us in debugging if we face issues with certain drives during kernel boot.
> We can disable features we think are causing issues and retry.

We should put this data into the soc_data struct in the driver, since it 
is easily determined from just the compatibility string; for example

struct tegra_ahci_soc {
	...
	uint32_t quirks;
};

then

.quirks = NO_DEVSLP | NO_DIPM

or the other way around and list the features that we want to enable. 
This is also quite easy to change to test.

The device tree should also only contain information that describes the 
hardware itself, and nothing related to any specific driver's operation; 
so if there's a possibility that some feature is not working due to a 
driver issue, then the device tree definitely should not contain any 
entry disabling that feature.

(The downstream kernel can of course carry such features as patches on 
top, if required for some reason - but naturally it increases 
maintenance effort)

Thanks,
Mikko.

RE: [v2,2/3] ata: ahci_tegra: Add support to disable features

[Preetham Chandru]

>-----Original Message-----
>From: Mikko Perttunen [mailto:cyndis@kapsi.fi]
>Sent: Wednesday, December 21, 2016 3:01 PM
>To: Preetham Chandru; Mikko Perttunen; tj@kernel.org;
>swarren@wwwdotorg.org; thierry.reding@gmail.com;
>preetham260@gmail.com
>Cc: Laxman Dewangan; linux-ide@vger.kernel.org; Venu Byravarasu; Pavan
>Kunapuli; linux-tegra@vger.kernel.org
>Subject: Re: [v2,2/3] ata: ahci_tegra: Add support to disable features
>
>On 21.12.2016 11:12, Preetham Chandru wrote:
>>
>>
>>> -----Original Message-----
>>> From: Mikko Perttunen
>>> Sent: Monday, November 28, 2016 6:09 PM
>>> To: Preetham Chandru; tj@kernel.org; swarren@wwwdotorg.org;
>>> thierry.reding@gmail.com; preetham260@gmail.com
>>> Cc: Laxman Dewangan; linux-ide@vger.kernel.org; Venu Byravarasu;
>>> Pavan Kunapuli; linux-tegra@vger.kernel.org
>>> Subject: Re: [v2,2/3] ata: ahci_tegra: Add support to disable
>>> features
>>>
>>> On 24.11.2016 09:43, PREETHAM RAMACHANDRA wrote:
>>>> From: Preetham Chandru R <pchandru@nvidia.com>
>>>>
>>>> Add support to disable DIPM, HIPM, DevSlp, partial, slumber and NCQ
>>>> features from DT. By default these features are enabled.
>>>>
>>>
>>> Why are these features disabled? Are they broken on all Tegra210
>>> chips, broken on specific boards or do they require some support from
>>> the driver that is not there?
>>>
>>> Most likely we should hardcode the disabled features into the driver
>>> instead of reading them from the device tree.
>>>
>>
>> Yes, currently DevSlp and DIPM features are broken for t210 and t124 but
>devslp is fixed for tegra186.
>>  Also I thought it would be nice to provide similar options for other
>features as well.
>> Since we do not support hotplug this would help us in debugging if we face
>issues with certain drives during kernel boot.
>> We can disable features we think are causing issues and retry.
>
>We should put this data into the soc_data struct in the driver, since it is
>easily determined from just the compatibility string; for example
>
>struct tegra_ahci_soc {
>	...
>	uint32_t quirks;
>};
>
>then
>
>.quirks = NO_DEVSLP | NO_DIPM
>
>or the other way around and list the features that we want to enable.
>This is also quite easy to change to test.
>
>The device tree should also only contain information that describes the
>hardware itself, and nothing related to any specific driver's operation; so if
>there's a possibility that some feature is not working due to a driver issue,
>then the device tree definitely should not contain any entry disabling that
>feature.
>
>(The downstream kernel can of course carry such features as patches on top,
>if required for some reason - but naturally it increases maintenance effort)
>

Ok, sounds good to me. Will move it to soc_data structure.

>Thanks,
>Mikko.

[PATCH v2 3/3] dt-bindings: ata: ahci_tegra: Add tegra210 AHCI

[Preetham Chandru Ramchandra]

From: Preetham Chandru R <pchandru@nvidia.com>

Signed-off-by: Preetham Chandru R <pchandru@nvidia.com>
---
 .../bindings/ata/nvidia,tegra124-ahci.txt          | 48 ++++++++++++++++------
 1 file changed, 36 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
index 66c83c3..446214f 100644
--- a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
+++ b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
@@ -1,9 +1,9 @@
-Tegra124 SoC SATA AHCI controller
+Tegra SoC SATA AHCI controller
 
 Required properties :
-- compatible : For Tegra124, must contain "nvidia,tegra124-ahci".  Otherwise,
-  must contain '"nvidia,<chip>-ahci", "nvidia,tegra124-ahci"', where <chip>
-  is tegra132.
+- compatible : Must be one of:
+  - Tegra124 : "nvidia,tegra124-ahci"
+  - Tegra210 : "nvidia,tegra210-ahci"
 - reg : Should contain 2 entries:
   - AHCI register set (SATA BAR5)
   - SATA register set
@@ -13,8 +13,6 @@ Required properties :
 - 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:
@@ -24,9 +22,35 @@ Required properties :
 - 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
+  - For T124:
+    - sata-phy : XUSB PADCTL SATA PHY
+  - For T210:
+    - sata-0
+- For T124:
+  - 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
+- For T210:
+  - l0-hvddio-sata-supply : Defines the SATA HVDDIO regulator
+  - l0-dvddio-sata-supply : Defines the SATA DVDDIO regulator
+  - hvdd-pex-pll-e-supply : Defines the PEX PLL_E regulator
+  - dvdd-sata-pll-supply  : Defines the SATA PLL regulator
+  - hvdd-sata-supply      : Defines the SATA HVDD regulator
+- nvidia,disable-features : Must include the following entries:
+  - devslp
+  - dipm
+
+Optional properties:
+- clock-names :
+  - cml1 :
+    cml1 clock is required by phy so it is optional to define
+    here as phy driver will be enabling this clock.
+  - pll_e :
+    pll_e is the parent of cml1 clock so it is optional to define
+    here as phy driver will be enabling this clock.
+- nvidia,disable-features :
+  - hipm
+  - ncq
+  - partial
+  - slumber
-- 
2.1.4

Re: [v2,3/3] dt-bindings: ata: ahci_tegra: Add tegra210 AHCI

[Mikko Perttunen]

On 24.11.2016 09:43, PREETHAM RAMACHANDRA wrote:
> From: Preetham Chandru R <pchandru-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> Signed-off-by: Preetham Chandru R <pchandru-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  .../bindings/ata/nvidia,tegra124-ahci.txt          | 48 ++++++++++++++++------
>  1 file changed, 36 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
> index 66c83c3..446214f 100644
> --- a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
> +++ b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
> @@ -1,9 +1,9 @@
> -Tegra124 SoC SATA AHCI controller
> +Tegra SoC SATA AHCI controller
>
>  Required properties :
> -- compatible : For Tegra124, must contain "nvidia,tegra124-ahci".  Otherwise,
> -  must contain '"nvidia,<chip>-ahci", "nvidia,tegra124-ahci"', where <chip>
> -  is tegra132.
> +- compatible : Must be one of:
> +  - Tegra124 : "nvidia,tegra124-ahci"
> +  - Tegra210 : "nvidia,tegra210-ahci"
>  - reg : Should contain 2 entries:
>    - AHCI register set (SATA BAR5)
>    - SATA register set

I think you added a new set in the first patch, AUX; you should add it 
here as well.

> @@ -13,8 +13,6 @@ Required properties :
>  - 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:
> @@ -24,9 +22,35 @@ Required properties :
>  - 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
> +  - For T124:
> +    - sata-phy : XUSB PADCTL SATA PHY
> +  - For T210:
> +    - sata-0
> +- For T124:
> +  - 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
> +- For T210:
> +  - l0-hvddio-sata-supply : Defines the SATA HVDDIO regulator
> +  - l0-dvddio-sata-supply : Defines the SATA DVDDIO regulator
> +  - hvdd-pex-pll-e-supply : Defines the PEX PLL_E regulator
> +  - dvdd-sata-pll-supply  : Defines the SATA PLL regulator
> +  - hvdd-sata-supply      : Defines the SATA HVDD regulator
> +- nvidia,disable-features : Must include the following entries:
> +  - devslp
> +  - dipm
> +
> +Optional properties:
> +- clock-names :
> +  - cml1 :
> +    cml1 clock is required by phy so it is optional to define
> +    here as phy driver will be enabling this clock.

The current phy driver does not seem to be enabling this clock. For 
PCIE, the PCIE driver itself enables cml0. It might be true that the phy 
driver should be enabling this. Thierry?

> +  - pll_e :
> +    pll_e is the parent of cml1 clock so it is optional to define
> +    here as phy driver will be enabling this clock.
> +- nvidia,disable-features :
> +  - hipm
> +  - ncq
> +  - partial
> +  - slumber
>
>

Thanks,
Mikko.

Re: [v2,3/3] dt-bindings: ata: ahci_tegra: Add tegra210 AHCI

[Jon Hunter]

On 28/11/16 13:05, Mikko Perttunen wrote:
> On 24.11.2016 09:43, PREETHAM RAMACHANDRA wrote:
>> From: Preetham Chandru R <pchandru-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

I did not receive the original (please CC linux-tegra as well if you did
not originally), but there should be some description here.

>> Signed-off-by: Preetham Chandru R <pchandru-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  .../bindings/ata/nvidia,tegra124-ahci.txt          | 48
>> ++++++++++++++++------
>>  1 file changed, 36 insertions(+), 12 deletions(-)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
>> b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
>> index 66c83c3..446214f 100644
>> --- a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
>> +++ b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
>> @@ -1,9 +1,9 @@
>> -Tegra124 SoC SATA AHCI controller
>> +Tegra SoC SATA AHCI controller
>>
>>  Required properties :
>> -- compatible : For Tegra124, must contain "nvidia,tegra124-ahci". 
>> Otherwise,
>> -  must contain '"nvidia,<chip>-ahci", "nvidia,tegra124-ahci"', where
>> <chip>
>> -  is tegra132.
>> +- compatible : Must be one of:
>> +  - Tegra124 : "nvidia,tegra124-ahci"
>> +  - Tegra210 : "nvidia,tegra210-ahci"
>>  - reg : Should contain 2 entries:
>>    - AHCI register set (SATA BAR5)
>>    - SATA register set
> 
> I think you added a new set in the first patch, AUX; you should add it
> here as well.
> 
>> @@ -13,8 +13,6 @@ Required properties :
>>  - 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:
>> @@ -24,9 +22,35 @@ Required properties :
>>  - 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
>> +  - For T124:
>> +    - sata-phy : XUSB PADCTL SATA PHY
>> +  - For T210:
>> +    - sata-0
>> +- For T124:
>> +  - 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
>> +- For T210:
>> +  - l0-hvddio-sata-supply : Defines the SATA HVDDIO regulator
>> +  - l0-dvddio-sata-supply : Defines the SATA DVDDIO regulator
>> +  - hvdd-pex-pll-e-supply : Defines the PEX PLL_E regulator
>> +  - dvdd-sata-pll-supply  : Defines the SATA PLL regulator
>> +  - hvdd-sata-supply      : Defines the SATA HVDD regulator
>> +- nvidia,disable-features : Must include the following entries:
>> +  - devslp
>> +  - dipm

My understanding is that the AHCI controller requires the SATA powergate
to be enabled. Now we have support for powergates via the genpd
framework we should add the 'power-domains' property for this device.

Do you know if there is any sequencing requirement with regard to
powering on the above rails and the powergate? If not we should check as
we should ensure that we have the proper sequencing.

Cheers
Jon

-- 
nvpublic

RE: [v2,3/3] dt-bindings: ata: ahci_tegra: Add tegra210 AHCI

[Preetham Chandru]

>-----Original Message-----
>From: Jonathan Hunter
>Sent: Monday, November 28, 2016 7:22 PM
>To: Mikko Perttunen; Preetham Chandru; tj@kernel.org;
>swarren@wwwdotorg.org; thierry.reding@gmail.com;
>preetham260@gmail.com
>Cc: Laxman Dewangan; linux-ide@vger.kernel.org; Venu Byravarasu; Pavan
>Kunapuli; linux-tegra@vger.kernel.org
>Subject: Re: [v2,3/3] dt-bindings: ata: ahci_tegra: Add tegra210 AHCI
>
>
>On 28/11/16 13:05, Mikko Perttunen wrote:
>> On 24.11.2016 09:43, PREETHAM RAMACHANDRA wrote:
>>> From: Preetham Chandru R <pchandru@nvidia.com>
>
>I did not receive the original (please CC linux-tegra as well if you did not
>originally), but there should be some description here.
>
>>> Signed-off-by: Preetham Chandru R <pchandru@nvidia.com>
>>> ---
>>>  .../bindings/ata/nvidia,tegra124-ahci.txt          | 48
>>> ++++++++++++++++------
>>>  1 file changed, 36 insertions(+), 12 deletions(-)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
>>> b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
>>> index 66c83c3..446214f 100644
>>> --- a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
>>> +++ b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
>>> @@ -1,9 +1,9 @@
>>> -Tegra124 SoC SATA AHCI controller
>>> +Tegra SoC SATA AHCI controller
>>>
>>>  Required properties :
>>> -- compatible : For Tegra124, must contain "nvidia,tegra124-ahci".
>>> Otherwise,
>>> -  must contain '"nvidia,<chip>-ahci", "nvidia,tegra124-ahci"', where
>>> <chip>
>>> -  is tegra132.
>>> +- compatible : Must be one of:
>>> +  - Tegra124 : "nvidia,tegra124-ahci"
>>> +  - Tegra210 : "nvidia,tegra210-ahci"
>>>  - reg : Should contain 2 entries:
>>>    - AHCI register set (SATA BAR5)
>>>    - SATA register set
>>
>> I think you added a new set in the first patch, AUX; you should add it
>> here as well.
>>
>>> @@ -13,8 +13,6 @@ Required properties :
>>>  - 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:
>>> @@ -24,9 +22,35 @@ Required properties :
>>>  - 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
>>> +  - For T124:
>>> +    - sata-phy : XUSB PADCTL SATA PHY
>>> +  - For T210:
>>> +    - sata-0
>>> +- For T124:
>>> +  - 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
>>> +- For T210:
>>> +  - l0-hvddio-sata-supply : Defines the SATA HVDDIO regulator
>>> +  - l0-dvddio-sata-supply : Defines the SATA DVDDIO regulator
>>> +  - hvdd-pex-pll-e-supply : Defines the PEX PLL_E regulator
>>> +  - dvdd-sata-pll-supply  : Defines the SATA PLL regulator
>>> +  - hvdd-sata-supply      : Defines the SATA HVDD regulator
>>> +- nvidia,disable-features : Must include the following entries:
>>> +  - devslp
>>> +  - dipm
>
>My understanding is that the AHCI controller requires the SATA powergate
>to be enabled. Now we have support for powergates via the genpd
>framework we should add the 'power-domains' property for this device.
>
>Do you know if there is any sequencing requirement with regard to powering
>on the above rails and the powergate? If not we should check as we should
>ensure that we have the proper sequencing.
>

All the regulators mentioned under T210 section are related to phy and they need to be enabled first.  
I will add the power-domain support for AHCI in a different patch. 

>Cheers
>Jon
>
>--
>nvpublic

RE: [v2,3/3] dt-bindings: ata: ahci_tegra: Add tegra210 AHCI

[Preetham Chandru]

>-----Original Message-----
>From: Mikko Perttunen
>Sent: Monday, November 28, 2016 6:35 PM
>To: Preetham Chandru; tj@kernel.org; swarren@wwwdotorg.org;
>thierry.reding@gmail.com; preetham260@gmail.com
>Cc: Laxman Dewangan; linux-ide@vger.kernel.org; Venu Byravarasu; Pavan
>Kunapuli; linux-tegra@vger.kernel.org
>Subject: Re: [v2,3/3] dt-bindings: ata: ahci_tegra: Add tegra210 AHCI
>
>On 24.11.2016 09:43, PREETHAM RAMACHANDRA wrote:
>> From: Preetham Chandru R <pchandru@nvidia.com>
>>
>> Signed-off-by: Preetham Chandru R <pchandru@nvidia.com>
>> ---
>>  .../bindings/ata/nvidia,tegra124-ahci.txt          | 48 ++++++++++++++++----
>--
>>  1 file changed, 36 insertions(+), 12 deletions(-)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
>> b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
>> index 66c83c3..446214f 100644
>> --- a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
>> +++ b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
>> @@ -1,9 +1,9 @@
>> -Tegra124 SoC SATA AHCI controller
>> +Tegra SoC SATA AHCI controller
>>
>>  Required properties :
>> -- compatible : For Tegra124, must contain "nvidia,tegra124-ahci".
>> Otherwise,
>> -  must contain '"nvidia,<chip>-ahci", "nvidia,tegra124-ahci"', where
>> <chip>
>> -  is tegra132.
>> +- compatible : Must be one of:
>> +  - Tegra124 : "nvidia,tegra124-ahci"
>> +  - Tegra210 : "nvidia,tegra210-ahci"
>>  - reg : Should contain 2 entries:
>>    - AHCI register set (SATA BAR5)
>>    - SATA register set
>
>I think you added a new set in the first patch, AUX; you should add it here as
>well.
>

ok

>> @@ -13,8 +13,6 @@ Required properties :
>>  - 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:
>> @@ -24,9 +22,35 @@ Required properties :
>>  - 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
>> +  - For T124:
>> +    - sata-phy : XUSB PADCTL SATA PHY
>> +  - For T210:
>> +    - sata-0
>> +- For T124:
>> +  - 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
>> +- For T210:
>> +  - l0-hvddio-sata-supply : Defines the SATA HVDDIO regulator
>> +  - l0-dvddio-sata-supply : Defines the SATA DVDDIO regulator
>> +  - hvdd-pex-pll-e-supply : Defines the PEX PLL_E regulator
>> +  - dvdd-sata-pll-supply  : Defines the SATA PLL regulator
>> +  - hvdd-sata-supply      : Defines the SATA HVDD regulator
>> +- nvidia,disable-features : Must include the following entries:
>> +  - devslp
>> +  - dipm
>> +
>> +Optional properties:
>> +- clock-names :
>> +  - cml1 :
>> +    cml1 clock is required by phy so it is optional to define
>> +    here as phy driver will be enabling this clock.
>
>The current phy driver does not seem to be enabling this clock. For PCIE, the
>PCIE driver itself enables cml0. It might be true that the phy driver should be
>enabling this. Thierry?
>

Yes, this should really be done in phy driver. I cross checked and I see phy driver is not enabling cml1 clk.
I will check with Thierry regarding this.  Depending on where we decide to enable cml1 clock  I will either
remove the optional properties or move it must include entries.

>> +  - pll_e :
>> +    pll_e is the parent of cml1 clock so it is optional to define
>> +    here as phy driver will be enabling this clock.
>> +- nvidia,disable-features :
>> +  - hipm
>> +  - ncq
>> +  - partial
>> +  - slumber
>>
>>
>
>Thanks,
>Mikko.

Thanks,
Preetham.