[PATCH v4 0/3] Add AHCI support for Tegra186

[PATCH v4 0/3] Add AHCI support for Tegra186

[Sowjanya Komatineni]

Re-sending dt-binding and ahci_tegra driver patches as v4 as device
tree patches from v3 are merged but not the AHCI Tegra driver.

Missed to add Jens Axboe to mailing list in v3. Adding for v4.

This series adds support for AHCI-compliant SATA to Tegra186 SoC.

This series includes patches for
- Converting text based dt-binding document to YAML.
- Adding dt-bindings for Tegra186.
- Adding Tegra186 support to Tegra AHCI driver.

Delta between patch versions:
[v4]:	Same as v3 except removed device tree patches as they are
	merged.
[v3]:	fixed yaml example to pass dt_binding_check
[v2]:	v1 feedback related to yaml dt-binding.
	Removed conditional reset order in yaml and updated dts files
	to maintain same order for commonly available resets across
	Tegra124 thru Tegra186.


Sowjanya Komatineni (3):
  dt-bindings: ata: tegra: Convert binding documentation to YAML
  dt-binding: ata: tegra: Add dt-binding documentation for Tegra186
  ata: ahci_tegra: Add AHCI support for Tegra186

 .../devicetree/bindings/ata/nvidia,tegra-ahci.yaml | 176 +++++++++++++++++++++
 .../bindings/ata/nvidia,tegra124-ahci.txt          |  44 ------
 drivers/ata/ahci_tegra.c                           |  60 +++++--
 3 files changed, 223 insertions(+), 57 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/ata/nvidia,tegra-ahci.yaml
 delete mode 100644 Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt

-- 
2.7.4

[PATCH v4 1/3] dt-bindings: ata: tegra: Convert binding documentation to YAML

[Sowjanya Komatineni]

This patch converts text based dt-binding document to YAML based
dt-binding document.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 .../devicetree/bindings/ata/nvidia,tegra-ahci.yaml | 138 +++++++++++++++++++++
 .../bindings/ata/nvidia,tegra124-ahci.txt          |  44 -------
 2 files changed, 138 insertions(+), 44 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/ata/nvidia,tegra-ahci.yaml
 delete mode 100644 Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt

diff --git a/Documentation/devicetree/bindings/ata/nvidia,tegra-ahci.yaml b/Documentation/devicetree/bindings/ata/nvidia,tegra-ahci.yaml
new file mode 100644
index 0000000..3c15aea
--- /dev/null
+++ b/Documentation/devicetree/bindings/ata/nvidia,tegra-ahci.yaml
@@ -0,0 +1,138 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/ata/nvidia,tegra-ahci.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Tegra AHCI SATA Controller
+
+maintainers:
+  - Thierry Reding <thierry.reding@gmail.com>
+  - Jonathan Hunter <jonathanh@nvidia.com>
+
+properties:
+  compatible:
+    enum:
+      - nvidia,tegra124-ahci
+      - nvidia,tegra132-ahci
+      - nvidia,tegra210-ahci
+
+  reg:
+    minItems: 2
+    maxItems: 3
+    items:
+      - description: AHCI registers
+      - description: SATA configuration and IPFS registers
+      - description: SATA AUX registers
+
+  interrupts:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: sata
+      - const: sata-oob
+
+  clocks:
+    maxItems: 2
+
+  reset-names:
+    items:
+      - const: sata
+      - const: sata-cold
+      - const: sata-oob
+
+  resets:
+    maxItems: 3
+
+  phy-names:
+    items:
+      - const: sata-0
+
+  phys:
+    maxItems: 1
+
+  hvdd-supply:
+    description: SATA HVDD regulator supply.
+
+  vddio-supply:
+    description: SATA VDDIO regulator supply.
+
+  avdd-supply:
+    description: SATA AVDD regulator supply.
+
+  target-5v-supply:
+    description: SATA 5V power regulator supply.
+
+  target-12v-supply:
+    description: SATA 12V power regulator supply.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clock-names
+  - clocks
+  - reset-names
+  - resets
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - nvidia,tegra124-ahci
+              - nvidia,tegra132-ahci
+    then:
+      properties:
+        reg:
+          maxItems: 2
+        reset-names:
+          minItems: 3
+        resets:
+          minItems: 3
+      required:
+        - phys
+        - phy-names
+        - hvdd-supply
+        - vddio-supply
+        - avdd-supply
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - nvidia,tegra210-ahci
+    then:
+      properties:
+        reg:
+          minItems: 3
+        reset-names:
+          minItems: 3
+        resets:
+          minItems: 3
+
+additionalProperties: true
+
+examples:
+  - |
+    #include <dt-bindings/clock/tegra210-car.h>
+    #include <dt-bindings/reset/tegra210-car.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    sata@70020000 {
+            compatible = "nvidia,tegra210-ahci";
+            reg = <0x70027000 0x00002000>, /* AHCI */
+                  <0x70020000 0x00007000>, /* SATA */
+                  <0x70001100 0x00010000>; /* SATA AUX */
+            interrupts = <GIC_SPI 23 IRQ_TYPE_LEVEL_HIGH>;
+            clocks = <&tegra_car TEGRA210_CLK_SATA>,
+                     <&tegra_car TEGRA210_CLK_SATA_OOB>;
+            clock-names = "sata", "sata-oob";
+            resets = <&tegra_car 124>,
+                     <&tegra_car 129>,
+                     <&tegra_car 123>;
+            reset-names = "sata", "sata-cold", "sata-oob";
+    };
diff --git a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
deleted file mode 100644
index 12ab2f7..0000000
--- a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
+++ /dev/null
@@ -1,44 +0,0 @@
-Tegra SoC SATA AHCI controller
-
-Required properties :
-- compatible : Must be one of:
-  - Tegra124 : "nvidia,tegra124-ahci"
-  - Tegra132 : "nvidia,tegra132-ahci", "nvidia,tegra124-ahci"
-  - Tegra210 : "nvidia,tegra210-ahci"
-- reg : Should contain 2 entries:
-  - AHCI register set (SATA BAR5)
-  - SATA register set
-- interrupts : Defines the interrupt used by SATA
-- clocks : Must contain an entry for each entry in clock-names.
-  See ../clocks/clock-bindings.txt for details.
-- clock-names : Must include the following entries:
-  - sata
-  - sata-oob
-- resets : Must contain an entry for each entry in reset-names.
-  See ../reset/reset.txt for details.
-- reset-names : Must include the following entries:
-  - sata
-  - sata-oob
-  - sata-cold
-- phys : Must contain an entry for each entry in phy-names.
-  See ../phy/phy-bindings.txt for details.
-- phy-names : Must include the following entries:
-  - For Tegra124 and Tegra132:
-    - sata-phy : XUSB PADCTL SATA PHY
-- For Tegra124 and Tegra132:
-  - 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
-
-Optional properties:
-- reg :
-  - AUX register set
-- clock-names :
-  - cml1 :
-    cml1 clock should be defined here if the PHY driver
-    doesn't manage them. If it does, they should not be.
-- phy-names :
-  - For T210:
-    - sata-phy
-- 
2.7.4

[PATCH v4 2/3] dt-binding: ata: tegra: Add dt-binding documentation for Tegra186

[Sowjanya Komatineni]

This patch adds dt-bindings documentation for Tegra186 AHCI
controller.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 .../devicetree/bindings/ata/nvidia,tegra-ahci.yaml | 38 ++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/Documentation/devicetree/bindings/ata/nvidia,tegra-ahci.yaml b/Documentation/devicetree/bindings/ata/nvidia,tegra-ahci.yaml
index 3c15aea..a75e9a8 100644
--- a/Documentation/devicetree/bindings/ata/nvidia,tegra-ahci.yaml
+++ b/Documentation/devicetree/bindings/ata/nvidia,tegra-ahci.yaml
@@ -16,6 +16,7 @@ properties:
       - nvidia,tegra124-ahci
       - nvidia,tegra132-ahci
       - nvidia,tegra210-ahci
+      - nvidia,tegra186-ahci
 
   reg:
     minItems: 2
@@ -37,14 +38,31 @@ properties:
     maxItems: 2
 
   reset-names:
+    minItems: 2
     items:
       - const: sata
       - const: sata-cold
       - const: sata-oob
 
   resets:
+    minItems: 2
     maxItems: 3
 
+  iommus:
+    maxItems: 1
+
+  interconnect-names:
+    items:
+      - const: dma-mem
+      - const: write
+
+  interconnects:
+    maxItems: 2
+
+  power-domains:
+    items:
+      - description: SAX power-domain
+
   phy-names:
     items:
       - const: sata-0
@@ -114,6 +132,26 @@ allOf:
         resets:
           minItems: 3
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - nvidia,tegra186-ahci
+    then:
+      properties:
+        reg:
+          minItems: 3
+        reset-names:
+          maxItems: 2
+        resets:
+          maxItems: 2
+      required:
+        - iommus
+        - interconnect-names
+        - interconnects
+        - power-domains
+
 additionalProperties: true
 
 examples:
-- 
2.7.4

[PATCH v4 3/3] ata: ahci_tegra: Add AHCI support for Tegra186

[Sowjanya Komatineni]

This patch adds support for AHCI-compliant Serial ATA controller
on Tegra186 SoC.

Tegra186 does not have sata-oob reset.
Tegra186 SATA_NVOOB register filed COMMA_CNT position and width are
different compared to Tegra210 and prior.

So, this patch adds a flag has_sata_oob_rst and tegra_ahci_regs to
SoC specific strcuture tegra_ahci_soc and updated their implementation
accordingly.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 drivers/ata/ahci_tegra.c | 60 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 47 insertions(+), 13 deletions(-)

diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c
index cb55ebc1..56612af 100644
--- a/drivers/ata/ahci_tegra.c
+++ b/drivers/ata/ahci_tegra.c
@@ -59,8 +59,6 @@
 #define T_SATA0_CFG_PHY_1_PAD_PLL_IDDQ_EN		BIT(22)
 
 #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)
@@ -154,11 +152,18 @@ struct tegra_ahci_ops {
 	int (*init)(struct ahci_host_priv *hpriv);
 };
 
+struct tegra_ahci_regs {
+	unsigned int nvoob_comma_cnt_mask;
+	unsigned int nvoob_comma_cnt_val;
+};
+
 struct tegra_ahci_soc {
 	const char *const		*supply_names;
 	u32				num_supplies;
 	bool				supports_devslp;
+	bool				has_sata_oob_rst;
 	const struct tegra_ahci_ops	*ops;
+	const struct tegra_ahci_regs	*regs;
 };
 
 struct tegra_ahci_priv {
@@ -240,11 +245,13 @@ static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
 	if (ret)
 		return ret;
 
-	ret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_SATA,
-						tegra->sata_clk,
-						tegra->sata_rst);
-	if (ret)
-		goto disable_regulators;
+	if (!tegra->pdev->dev.pm_domain) {
+		ret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_SATA,
+							tegra->sata_clk,
+							tegra->sata_rst);
+		if (ret)
+			goto disable_regulators;
+	}
 
 	reset_control_assert(tegra->sata_oob_rst);
 	reset_control_assert(tegra->sata_cold_rst);
@@ -330,10 +337,10 @@ static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv)
 	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA_CFG_PHY_0);
 
 	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_NVOOB);
-	val &= ~(T_SATA0_NVOOB_COMMA_CNT_MASK |
+	val &= ~(tegra->soc->regs->nvoob_comma_cnt_mask |
 		 T_SATA0_NVOOB_SQUELCH_FILTER_LENGTH_MASK |
 		 T_SATA0_NVOOB_SQUELCH_FILTER_MODE_MASK);
-	val |= (T_SATA0_NVOOB_COMMA_CNT |
+	val |= (tegra->soc->regs->nvoob_comma_cnt_val |
 		T_SATA0_NVOOB_SQUELCH_FILTER_LENGTH |
 		T_SATA0_NVOOB_SQUELCH_FILTER_MODE);
 	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_NVOOB);
@@ -449,15 +456,35 @@ static const struct tegra_ahci_ops tegra124_ahci_ops = {
 	.init = tegra124_ahci_init,
 };
 
+static const struct tegra_ahci_regs tegra124_ahci_regs = {
+	.nvoob_comma_cnt_mask = GENMASK(30, 28),
+	.nvoob_comma_cnt_val = (7 << 28),
+};
+
 static const struct tegra_ahci_soc tegra124_ahci_soc = {
 	.supply_names = tegra124_supply_names,
 	.num_supplies = ARRAY_SIZE(tegra124_supply_names),
 	.supports_devslp = false,
+	.has_sata_oob_rst = true,
 	.ops = &tegra124_ahci_ops,
+	.regs = &tegra124_ahci_regs,
 };
 
 static const struct tegra_ahci_soc tegra210_ahci_soc = {
 	.supports_devslp = false,
+	.has_sata_oob_rst = true,
+	.regs = &tegra124_ahci_regs,
+};
+
+static const struct tegra_ahci_regs tegra186_ahci_regs = {
+	.nvoob_comma_cnt_mask = GENMASK(23, 16),
+	.nvoob_comma_cnt_val = (7 << 16),
+};
+
+static const struct tegra_ahci_soc tegra186_ahci_soc = {
+	.supports_devslp = false,
+	.has_sata_oob_rst = false,
+	.regs = &tegra186_ahci_regs,
 };
 
 static const struct of_device_id tegra_ahci_of_match[] = {
@@ -469,6 +496,10 @@ static const struct of_device_id tegra_ahci_of_match[] = {
 		.compatible = "nvidia,tegra210-ahci",
 		.data = &tegra210_ahci_soc
 	},
+	{
+		.compatible = "nvidia,tegra186-ahci",
+		.data = &tegra186_ahci_soc
+	},
 	{}
 };
 MODULE_DEVICE_TABLE(of, tegra_ahci_of_match);
@@ -518,10 +549,13 @@ static int tegra_ahci_probe(struct platform_device *pdev)
 		return PTR_ERR(tegra->sata_rst);
 	}
 
-	tegra->sata_oob_rst = devm_reset_control_get(&pdev->dev, "sata-oob");
-	if (IS_ERR(tegra->sata_oob_rst)) {
-		dev_err(&pdev->dev, "Failed to get sata-oob reset\n");
-		return PTR_ERR(tegra->sata_oob_rst);
+	if (tegra->soc->has_sata_oob_rst) {
+		tegra->sata_oob_rst = devm_reset_control_get(&pdev->dev,
+							     "sata-oob");
+		if (IS_ERR(tegra->sata_oob_rst)) {
+			dev_err(&pdev->dev, "Failed to get sata-oob reset\n");
+			return PTR_ERR(tegra->sata_oob_rst);
+		}
 	}
 
 	tegra->sata_cold_rst = devm_reset_control_get(&pdev->dev, "sata-cold");
-- 
2.7.4

Re: [PATCH v4 1/3] dt-bindings: ata: tegra: Convert binding documentation to YAML

[Thierry Reding]

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

On Tue, Apr 06, 2021 at 06:25:29PM -0700, Sowjanya Komatineni wrote:
> This patch converts text based dt-binding document to YAML based
> dt-binding document.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  .../devicetree/bindings/ata/nvidia,tegra-ahci.yaml | 138 +++++++++++++++++++++
>  .../bindings/ata/nvidia,tegra124-ahci.txt          |  44 -------
>  2 files changed, 138 insertions(+), 44 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/ata/nvidia,tegra-ahci.yaml
>  delete mode 100644 Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 2/3] dt-binding: ata: tegra: Add dt-binding documentation for Tegra186

[Thierry Reding]

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

On Tue, Apr 06, 2021 at 06:25:30PM -0700, Sowjanya Komatineni wrote:
> This patch adds dt-bindings documentation for Tegra186 AHCI
> controller.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  .../devicetree/bindings/ata/nvidia,tegra-ahci.yaml | 38 ++++++++++++++++++++++
>  1 file changed, 38 insertions(+)

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 3/3] ata: ahci_tegra: Add AHCI support for Tegra186

[Thierry Reding]

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

On Tue, Apr 06, 2021 at 06:25:31PM -0700, Sowjanya Komatineni wrote:
> This patch adds support for AHCI-compliant Serial ATA controller
> on Tegra186 SoC.
> 
> Tegra186 does not have sata-oob reset.
> Tegra186 SATA_NVOOB register filed COMMA_CNT position and width are
> different compared to Tegra210 and prior.
> 
> So, this patch adds a flag has_sata_oob_rst and tegra_ahci_regs to
> SoC specific strcuture tegra_ahci_soc and updated their implementation
> accordingly.
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  drivers/ata/ahci_tegra.c | 60 +++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 47 insertions(+), 13 deletions(-)

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 0/3] Add AHCI support for Tegra186

[Jens Axboe]

On 4/6/21 7:25 PM, Sowjanya Komatineni wrote:
> Re-sending dt-binding and ahci_tegra driver patches as v4 as device
> tree patches from v3 are merged but not the AHCI Tegra driver.
> 
> Missed to add Jens Axboe to mailing list in v3. Adding for v4.
> 
> This series adds support for AHCI-compliant SATA to Tegra186 SoC.
> 
> This series includes patches for
> - Converting text based dt-binding document to YAML.
> - Adding dt-bindings for Tegra186.
> - Adding Tegra186 support to Tegra AHCI driver.
> 
> Delta between patch versions:
> [v4]:	Same as v3 except removed device tree patches as they are
> 	merged.
> [v3]:	fixed yaml example to pass dt_binding_check
> [v2]:	v1 feedback related to yaml dt-binding.
> 	Removed conditional reset order in yaml and updated dts files
> 	to maintain same order for commonly available resets across
> 	Tegra124 thru Tegra186.

Assuming the libata tree is the best way for this to go in, so I applied
it for 5.13.

-- 
Jens Axboe

Re: [PATCH v4 0/3] Add AHCI support for Tegra186

[Thierry Reding]

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

On Wed, Apr 07, 2021 at 09:44:58AM -0600, Jens Axboe wrote:
> On 4/6/21 7:25 PM, Sowjanya Komatineni wrote:
> > Re-sending dt-binding and ahci_tegra driver patches as v4 as device
> > tree patches from v3 are merged but not the AHCI Tegra driver.
> > 
> > Missed to add Jens Axboe to mailing list in v3. Adding for v4.
> > 
> > This series adds support for AHCI-compliant SATA to Tegra186 SoC.
> > 
> > This series includes patches for
> > - Converting text based dt-binding document to YAML.
> > - Adding dt-bindings for Tegra186.
> > - Adding Tegra186 support to Tegra AHCI driver.
> > 
> > Delta between patch versions:
> > [v4]:	Same as v3 except removed device tree patches as they are
> > 	merged.
> > [v3]:	fixed yaml example to pass dt_binding_check
> > [v2]:	v1 feedback related to yaml dt-binding.
> > 	Removed conditional reset order in yaml and updated dts files
> > 	to maintain same order for commonly available resets across
> > 	Tegra124 thru Tegra186.
> 
> Assuming the libata tree is the best way for this to go in, so I applied
> it for 5.13.

Perfect! Thanks Jens.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 3/3] ata: ahci_tegra: Add AHCI support for Tegra186

[Dmitry Osipenko]

07.04.2021 04:25, Sowjanya Komatineni пишет:
> +	if (!tegra->pdev->dev.pm_domain) {
> +		ret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_SATA,
> +							tegra->sata_clk,
> +							tegra->sata_rst);
> +		if (ret)
> +			goto disable_regulators;
> +	}
>  

Hi,

Why you haven't added condition for tegra_powergate_power_off()? I think
it should break GENPD and legacy PD API isn't not supported by T186 at all.

I'm also not sure whether the power up/down sequence is correct using GENPD.

Moreover the driver doesn't support runtime PM, so GENPD should be
always off?

Re: [PATCH v4 3/3] ata: ahci_tegra: Add AHCI support for Tegra186

[Sowjanya Komatineni]

On 4/7/21 2:36 PM, Dmitry Osipenko wrote:
> 07.04.2021 04:25, Sowjanya Komatineni пишет:
>> +	if (!tegra->pdev->dev.pm_domain) {
>> +		ret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_SATA,
>> +							tegra->sata_clk,
>> +							tegra->sata_rst);
>> +		if (ret)
>> +			goto disable_regulators;
>> +	}
>>   
> Hi,
>
> Why you haven't added condition for tegra_powergate_power_off()? I think
> it should break GENPD and legacy PD API isn't not supported by T186 at all.
>
> I'm also not sure whether the power up/down sequence is correct using GENPD.
>
> Moreover the driver doesn't support runtime PM, so GENPD should be
> always off?

This driver already using legacy PD API's so thought its supported and 
added power domain device check during powergate_sequence_power_up and 
yes same should apply for powergate_power_off as well. But if legacy PD 
is not supported by T186 then not sure why original driver even using 
these API's.

Preetham/Thierry, Can you please comment ?

But as RPM is not implemented yet for this driver, GENPD will be OFF but 
SATA is not in power-gate by the time kernel starts and functionally works.

But with RPM implementation, I guess we can do proper power gate on/off.

Re: [PATCH v4 3/3] ata: ahci_tegra: Add AHCI support for Tegra186

[Sowjanya Komatineni]

On 4/7/21 3:57 PM, Sowjanya Komatineni wrote:
>
> On 4/7/21 2:36 PM, Dmitry Osipenko wrote:
>> 07.04.2021 04:25, Sowjanya Komatineni пишет:
>>> +    if (!tegra->pdev->dev.pm_domain) {
>>> +        ret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_SATA,
>>> +                            tegra->sata_clk,
>>> +                            tegra->sata_rst);
>>> +        if (ret)
>>> +            goto disable_regulators;
>>> +    }
>> Hi,
>>
>> Why you haven't added condition for tegra_powergate_power_off()? I think
>> it should break GENPD and legacy PD API isn't not supported by T186 
>> at all.
>>
>> I'm also not sure whether the power up/down sequence is correct using 
>> GENPD.
>>
>> Moreover the driver doesn't support runtime PM, so GENPD should be
>> always off?
>
> This driver already using legacy PD API's so thought its supported and 
> added power domain device check during powergate_sequence_power_up and 
> yes same should apply for powergate_power_off as well. But if legacy 
> PD is not supported by T186 then not sure why original driver even 
> using these API's.
>
>
Sorry just took a look and driver supports T210 and prior tegra as well. 
T210 and prior supports legacy PD and this check is applicable for 
those. So we should add power domain device check for power off as well.

But for T186, we should have GENPD working once we add runtime PM 
support to driver.

Preetham/Thierry, Can you confirm where SATA is un powergated prior to 
kernel?


> But as RPM is not implemented yet for this driver, GENPD will be OFF 
> but SATA is not in power-gate by the time kernel starts and 
> functionally works.
>
> But with RPM implementation, I guess we can do proper power gate on/off.
>

Re: [PATCH v4 3/3] ata: ahci_tegra: Add AHCI support for Tegra186

[Dmitry Osipenko]

08.04.2021 02:00, Sowjanya Komatineni пишет:
> 
> On 4/7/21 3:57 PM, Sowjanya Komatineni wrote:
>>
>> On 4/7/21 2:36 PM, Dmitry Osipenko wrote:
>>> 07.04.2021 04:25, Sowjanya Komatineni пишет:
>>>> +    if (!tegra->pdev->dev.pm_domain) {
>>>> +        ret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_SATA,
>>>> +                            tegra->sata_clk,
>>>> +                            tegra->sata_rst);
>>>> +        if (ret)
>>>> +            goto disable_regulators;
>>>> +    }
>>> Hi,
>>>
>>> Why you haven't added condition for tegra_powergate_power_off()? I think
>>> it should break GENPD and legacy PD API isn't not supported by T186
>>> at all.
>>>
>>> I'm also not sure whether the power up/down sequence is correct using
>>> GENPD.
>>>
>>> Moreover the driver doesn't support runtime PM, so GENPD should be
>>> always off?
>>
>> This driver already using legacy PD API's so thought its supported and
>> added power domain device check during powergate_sequence_power_up and
>> yes same should apply for powergate_power_off as well. But if legacy
>> PD is not supported by T186 then not sure why original driver even
>> using these API's.
>>
>>
> Sorry just took a look and driver supports T210 and prior tegra as well.
> T210 and prior supports legacy PD and this check is applicable for
> those. So we should add power domain device check for power off as well.

You could fix it with a follow up patch. Please try to test that
power-off works properly, at least try to unload the driver module and
re-load it.

> But for T186, we should have GENPD working once we add runtime PM
> support to driver.
> 
> Preetham/Thierry, Can you confirm where SATA is un powergated prior to
> kernel?
> 
> 
>> But as RPM is not implemented yet for this driver, GENPD will be OFF
>> but SATA is not in power-gate by the time kernel starts and
>> functionally works.
>>
>> But with RPM implementation, I guess we can do proper power gate on/off.
>>

I now recalled that GENPD turns ON all domains by default and then turns
them OFF only when driver entered into the RPM-suspended state. This
means that AHCI GENPD should be always-ON for T186, which should be okay
if this doesn't break power sequences.

Re: [PATCH v4 3/3] ata: ahci_tegra: Add AHCI support for Tegra186

[Thierry Reding]

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

On Thu, Apr 08, 2021 at 02:25:19AM +0300, Dmitry Osipenko wrote:
> 08.04.2021 02:00, Sowjanya Komatineni пишет:
> > 
> > On 4/7/21 3:57 PM, Sowjanya Komatineni wrote:
> >>
> >> On 4/7/21 2:36 PM, Dmitry Osipenko wrote:
> >>> 07.04.2021 04:25, Sowjanya Komatineni пишет:
> >>>> +    if (!tegra->pdev->dev.pm_domain) {
> >>>> +        ret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_SATA,
> >>>> +                            tegra->sata_clk,
> >>>> +                            tegra->sata_rst);
> >>>> +        if (ret)
> >>>> +            goto disable_regulators;
> >>>> +    }
> >>> Hi,
> >>>
> >>> Why you haven't added condition for tegra_powergate_power_off()? I think
> >>> it should break GENPD and legacy PD API isn't not supported by T186
> >>> at all.
> >>>
> >>> I'm also not sure whether the power up/down sequence is correct using
> >>> GENPD.
> >>>
> >>> Moreover the driver doesn't support runtime PM, so GENPD should be
> >>> always off?
> >>
> >> This driver already using legacy PD API's so thought its supported and
> >> added power domain device check during powergate_sequence_power_up and
> >> yes same should apply for powergate_power_off as well. But if legacy
> >> PD is not supported by T186 then not sure why original driver even
> >> using these API's.
> >>
> >>
> > Sorry just took a look and driver supports T210 and prior tegra as well.
> > T210 and prior supports legacy PD and this check is applicable for
> > those. So we should add power domain device check for power off as well.
> 
> You could fix it with a follow up patch. Please try to test that
> power-off works properly, at least try to unload the driver module and
> re-load it.

Agreed, this should have the same check as above for
tegra_powergate_power_off(). It currently works fine because on Tegra186
tegra_powergate_power_off() (and all the other legacy APIs for that
matter) will abort early since no power gates are implemented. The AHCI
driver doesn't check for errors, so this will just fail silently. It's
better to be symmetric, though, and add the check in both paths.

> > But for T186, we should have GENPD working once we add runtime PM
> > support to driver.
> > 
> > Preetham/Thierry, Can you confirm where SATA is un powergated prior to
> > kernel?
> > 
> > 
> >> But as RPM is not implemented yet for this driver, GENPD will be OFF
> >> but SATA is not in power-gate by the time kernel starts and
> >> functionally works.
> >>
> >> But with RPM implementation, I guess we can do proper power gate on/off.
> >>
> 
> I now recalled that GENPD turns ON all domains by default and then turns
> them OFF only when driver entered into the RPM-suspended state. This
> means that AHCI GENPD should be always-ON for T186, which should be okay
> if this doesn't break power sequences.

Yeah, the generic PM domain will just stay enabled after probe and until
remove. This does not impact the power sequences because they have to be
completely implemented in the power domains code anyway. With the legacy
API we used to need more rigorous sequences in the individual drivers,
but with generic PM domains none of that should be necessary, though it
also doesn't hurt, so some of the unnecessary clock enablement code is
kept for simplicity.

To be honest, I'm not sure if it's worth adding runtime PM support for
this driver. If this top-level layer has a way of getting notification
when no device was detected, then it might make some sense to turn off
the power domain and the regulators again, but I'm not sure if that's
the case. tegra_ahci_host_stop() seems like it might be usable for that
so yeah, that might work. We currently do turn off the powergate in that
case, so extending that power optimization to Tegra186 using runtime PM
makes sense.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 3/3] ata: ahci_tegra: Add AHCI support for Tegra186

[Dmitry Osipenko]

08.04.2021 16:06, Thierry Reding пишет:
> On Thu, Apr 08, 2021 at 02:25:19AM +0300, Dmitry Osipenko wrote:
>> 08.04.2021 02:00, Sowjanya Komatineni пишет:
>>>
>>> On 4/7/21 3:57 PM, Sowjanya Komatineni wrote:
>>>>
>>>> On 4/7/21 2:36 PM, Dmitry Osipenko wrote:
>>>>> 07.04.2021 04:25, Sowjanya Komatineni пишет:
>>>>>> +    if (!tegra->pdev->dev.pm_domain) {
>>>>>> +        ret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_SATA,
>>>>>> +                            tegra->sata_clk,
>>>>>> +                            tegra->sata_rst);
>>>>>> +        if (ret)
>>>>>> +            goto disable_regulators;
>>>>>> +    }
>>>>> Hi,
>>>>>
>>>>> Why you haven't added condition for tegra_powergate_power_off()? I think
>>>>> it should break GENPD and legacy PD API isn't not supported by T186
>>>>> at all.
>>>>>
>>>>> I'm also not sure whether the power up/down sequence is correct using
>>>>> GENPD.
>>>>>
>>>>> Moreover the driver doesn't support runtime PM, so GENPD should be
>>>>> always off?
>>>>
>>>> This driver already using legacy PD API's so thought its supported and
>>>> added power domain device check during powergate_sequence_power_up and
>>>> yes same should apply for powergate_power_off as well. But if legacy
>>>> PD is not supported by T186 then not sure why original driver even
>>>> using these API's.
>>>>
>>>>
>>> Sorry just took a look and driver supports T210 and prior tegra as well.
>>> T210 and prior supports legacy PD and this check is applicable for
>>> those. So we should add power domain device check for power off as well.
>>
>> You could fix it with a follow up patch. Please try to test that
>> power-off works properly, at least try to unload the driver module and
>> re-load it.
> 
> Agreed, this should have the same check as above for
> tegra_powergate_power_off(). It currently works fine because on Tegra186
> tegra_powergate_power_off() (and all the other legacy APIs for that
> matter) will abort early since no power gates are implemented. The AHCI
> driver doesn't check for errors, so this will just fail silently. It's
> better to be symmetric, though, and add the check in both paths.

I missed that tegra_powergate_power_off() usage isn't fatal if GENPD is
used, thank you for the clarification.

>>> But for T186, we should have GENPD working once we add runtime PM
>>> support to driver.
>>>
>>> Preetham/Thierry, Can you confirm where SATA is un powergated prior to
>>> kernel?
>>>
>>>
>>>> But as RPM is not implemented yet for this driver, GENPD will be OFF
>>>> but SATA is not in power-gate by the time kernel starts and
>>>> functionally works.
>>>>
>>>> But with RPM implementation, I guess we can do proper power gate on/off.
>>>>
>>
>> I now recalled that GENPD turns ON all domains by default and then turns
>> them OFF only when driver entered into the RPM-suspended state. This
>> means that AHCI GENPD should be always-ON for T186, which should be okay
>> if this doesn't break power sequences.
> 
> Yeah, the generic PM domain will just stay enabled after probe and until
> remove. This does not impact the power sequences because they have to be
> completely implemented in the power domains code anyway. With the legacy
> API we used to need more rigorous sequences in the individual drivers,
> but with generic PM domains none of that should be necessary, though it
> also doesn't hurt, so some of the unnecessary clock enablement code is
> kept for simplicity.
> 
> To be honest, I'm not sure if it's worth adding runtime PM support for
> this driver. If this top-level layer has a way of getting notification
> when no device was detected, then it might make some sense to turn off
> the power domain and the regulators again, but I'm not sure if that's
> the case. tegra_ahci_host_stop() seems like it might be usable for that
> so yeah, that might work. We currently do turn off the powergate in that
> case, so extending that power optimization to Tegra186 using runtime PM
> makes sense.

Alright, then this all should be good as-is.