[PATCH 0/9] Serial ATA support for NVIDIA Tegra124

[PATCH 0/9] Serial ATA support for NVIDIA Tegra124

[Mikko Perttunen]

Hi,

This series adds support for the onboard AHCI-compliant Serial ATA
controller found on Tegra124 systems-on-chip. The controller is
enabled on Jetson TK1. The series depends on Peter's efuse series 
and Thierry's yet to be submitted XUSB pinctrl driver.

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

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

 .../devicetree/bindings/ata/tegra-sata.txt         |  29 ++
 arch/arm/boot/dts/tegra124-jetson-tk1.dts          |  36 ++
 arch/arm/boot/dts/tegra124.dtsi                    |  24 ++
 arch/arm/configs/tegra_defconfig                   |   3 +
 arch/arm/mach-tegra/powergate.c                    |   1 +
 drivers/ata/Kconfig                                |   9 +
 drivers/ata/Makefile                               |   1 +
 drivers/ata/ahci.h                                 |   2 +-
 drivers/ata/ahci_tegra.c                           | 386 +++++++++++++++++++++
 drivers/clk/tegra/clk-pll.c                        |   8 +
 drivers/clk/tegra/clk-tegra124.c                   |   2 +
 11 files changed, 500 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/ata/tegra-sata.txt
 create mode 100644 drivers/ata/ahci_tegra.c

-- 
1.8.1.5

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

[Mikko Perttunen]

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

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

diff --git a/Documentation/devicetree/bindings/ata/tegra-sata.txt b/Documentation/devicetree/bindings/ata/tegra-sata.txt
new file mode 100644
index 0000000..70e3119
--- /dev/null
+++ b/Documentation/devicetree/bindings/ata/tegra-sata.txt
@@ -0,0 +1,29 @@
+Tegra124 SoC SATA AHCI controller
+
+Required properties :
+ - compatible : Should be "nvidia,tegra124-ahci".
+ - reg : Defines the following set of registers, in the order listed:
+   - AHCI register set (SATA BAR5)
+   - SATA register set
+ - interrupts : Defines the interrupt used by SATA
+ - clocks : Defines the clocks listed in the clock-names property.
+ - clock-names : The following clock names must be present:
+   - sata
+   - sata-oob
+   - cml1
+   - pll_e
+ - resets : Defines the resets listed in the reset-names property
+ - reset-names : The following reset names must be present:
+   - sata
+   - sata-oob
+   - sata-cold
+ - phys : Defines the phys listed in the phy-names property
+ - phy-names : The following phy names must be present:
+   - 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
+
+Optional properties:
+ - target-supply : Defines a regulator for a power output connector
+                   for SATA devices.
-- 
1.8.1.5

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

[Stephen Warren]

On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> This patch adds device tree binding documentation for the SATA
> controller found on NVIDIA Tegra SoCs.

Just one nit below:

> diff --git a/Documentation/devicetree/bindings/ata/tegra-sata.txt b/Documentation/devicetree/bindings/ata/tegra-sata.txt

> + - clocks : Defines the clocks listed in the clock-names property.
> + - clock-names : The following clock names must be present:
> +   - sata
...

It would be nice if the binding could use the exact same wording as all
the other Tegra bindings, i.e.:

==========
- 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:
  - pll_a
  - pll_a_out0
  - mclk (The Tegra cdev1/extern1 clock, which feeds the CODEC's mclk)
==========

Same for the resets property.

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

[Mikko Perttunen]

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

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
Cc: devicetree at vger.kernel.org
---
 arch/arm/boot/dts/tegra124.dtsi | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

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

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

[Mikko Perttunen]

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

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
Cc: devicetree at vger.kernel.org
---
 arch/arm/boot/dts/tegra124-jetson-tk1.dts | 36 +++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

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

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

[Stephen Warren]

On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> This enables the integrated SATA controller on the Tegra124 system-on-chip
> on the Jetson TK1 board and adds regulators for the onboard Molex connector
> commonly used to power SATA devices. The regulators are marked always-on
> since they can be used for other purposes than powering SATA devices.

> diff --git a/arch/arm/boot/dts/tegra124-jetson-tk1.dts b/arch/arm/boot/dts/tegra124-jetson-tk1.dts

> +	sata at 0,70020000 {

> +		target-supply = <&vdd_5v0_sata>;

   	regulators {

> +		/* Molex power connector */
> +		vdd_5v0_sata: regulator at 13 {
> +			compatible = "regulator-fixed";
> +			reg = <13>;
> +			regulator-name = "+5V_SATA";
> +			regulator-min-microvolt = <5000000>;
> +			regulator-max-microvolt = <5000000>;
> +			gpio = <&gpio TEGRA_GPIO(EE, 2) GPIO_ACTIVE_HIGH>;
> +			enable-active-high;
> +			vin-supply = <&vdd_5v0_sys>;
> +			regulator-always-on;
> +		};

Why is this always-on, considering that the SATA node references this,
and presumably the driver explicitly enables this regulator?

> +
> +		vdd_12v0_sata: regulator at 14 {
> +			compatible = "regulator-fixed";
> +			reg = <14>;
> +			regulator-name = "+12V_SATA";
> +			regulator-min-microvolt = <12000000>;
> +			regulator-max-microvolt = <12000000>;
> +			gpio = <&gpio TEGRA_GPIO(EE, 2) GPIO_ACTIVE_HIGH>;
> +			enable-active-high;
> +			vin-supply = <&vdd_mux>;
> +			regulator-always-on;
> +		};
>  	};

If there are two regulators required for the SATA devices, shouldn't the
SATA node's target-supply property reference them both? Well, I suppose
there'd need to be target-5v-supply and target-12v-supply properties,
since each regulator property can only reference a single supply, IIRC.
I think the DT binding needs to be updated for this.

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

[Mikko Perttunen]

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

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 drivers/clk/tegra/clk-pll.c | 8 ++++++++
 1 file changed, 8 insertions(+)

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

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

[Stephen Warren]

On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> This makes the SATA PLL be controlled by hardware instead of software.
> This is required for working SATA support.

Peter, could you please take patches 4 and 5 through the clock tree. As
far as I can tell, there's no compile-time dependency in the clock
patches, so they can go through a different tree to the rest of the
series without issue. These 2 patches look fine to me, so consider them:

Acked-by: Stephen Warren <swarren@nvidia.com>

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

[Peter De Schrijver]

On Mon, Jun 16, 2014 at 11:49:18PM +0200, Stephen Warren wrote:
> On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> > This makes the SATA PLL be controlled by hardware instead of software.
> > This is required for working SATA support.
> 
> Peter, could you please take patches 4 and 5 through the clock tree. As
> far as I can tell, there's no compile-time dependency in the clock
> patches, so they can go through a different tree to the rest of the
> series without issue. These 2 patches look fine to me, so consider them:
> 
> Acked-by: Stephen Warren <swarren@nvidia.com>

Ok. Will do.

Cheers,

Peter.

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

[Mikko Perttunen]

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

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 drivers/clk/tegra/clk-tegra124.c | 2 ++
 1 file changed, 2 insertions(+)

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

[PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Mikko Perttunen]

This symbol needs to be exported to power on rails without using
tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
cannot be used in situations where the driver wants to handle clocking
by itself.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 arch/arm/mach-tegra/powergate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-tegra/powergate.c b/arch/arm/mach-tegra/powergate.c
index 50a9af4..254c42e 100644
--- a/arch/arm/mach-tegra/powergate.c
+++ b/arch/arm/mach-tegra/powergate.c
@@ -125,6 +125,7 @@ int tegra_powergate_power_on(int id)
 
 	return tegra_powergate_set(id, true);
 }
+EXPORT_SYMBOL(tegra_powergate_power_on);
 
 int tegra_powergate_power_off(int id)
 {
-- 
1.8.1.5

[PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Stephen Warren]

On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> This symbol needs to be exported to power on rails without using
> tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
> cannot be used in situations where the driver wants to handle clocking
> by itself.

Thierry, are you OK with this change?

[PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Thierry Reding]

On Mon, Jun 16, 2014 at 04:01:02PM -0600, Stephen Warren wrote:
> On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> > This symbol needs to be exported to power on rails without using
> > tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
> > cannot be used in situations where the driver wants to handle clocking
> > by itself.
> 
> Thierry, are you OK with this change?

I would've preferred tegra_powergate_sequence_power_up() to be used
consistently in all drivers. I'm still not convinced that using the
platform AHCI driver this way is really the best option, since we're
bending over backwards to fit into what this driver dictates. There
shouldn't be a need for that.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140617/621f2d7e/attachment.sig>

[PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Mikko Perttunen]

I'll try removing use of all libahci_platform stuff except 
ahci_platform_init_host for v2.

On 17/06/14 15:13, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Mon, Jun 16, 2014 at 04:01:02PM -0600, Stephen Warren wrote:
>> On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
>>> This symbol needs to be exported to power on rails without using
>>> tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
>>> cannot be used in situations where the driver wants to handle clocking
>>> by itself.
>>
>> Thierry, are you OK with this change?
>
> I would've preferred tegra_powergate_sequence_power_up() to be used
> consistently in all drivers. I'm still not convinced that using the
> platform AHCI driver this way is really the best option, since we're
> bending over backwards to fit into what this driver dictates. There
> shouldn't be a need for that.
>
> Thierry
>
> * Unknown Key
> * 0x7F3EB3A1
>

[PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Peter De Schrijver]

On Tue, Jun 17, 2014 at 02:13:15PM +0200, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Mon, Jun 16, 2014 at 04:01:02PM -0600, Stephen Warren wrote:
> > On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> > > This symbol needs to be exported to power on rails without using
> > > tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
> > > cannot be used in situations where the driver wants to handle clocking
> > > by itself.
> > 
> > Thierry, are you OK with this change?
> 
> I would've preferred tegra_powergate_sequence_power_up() to be used

I don't think the current tegra_powergate_sequence_power_up() API is very well
defined though. I don't think the clocks and resets required by the sequence
should be provided by the driver. For one, there can be several clocks and
resets that need to be controlled for a single domain.

Cheers,

Peter.

[PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Thierry Reding]

On Tue, Jun 17, 2014 at 05:01:46PM +0300, Peter De Schrijver wrote:
> On Tue, Jun 17, 2014 at 02:13:15PM +0200, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Mon, Jun 16, 2014 at 04:01:02PM -0600, Stephen Warren wrote:
> > > On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> > > > This symbol needs to be exported to power on rails without using
> > > > tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
> > > > cannot be used in situations where the driver wants to handle clocking
> > > > by itself.
> > > 
> > > Thierry, are you OK with this change?
> > 
> > I would've preferred tegra_powergate_sequence_power_up() to be used
> 
> I don't think the current tegra_powergate_sequence_power_up() API is very well
> defined though. I don't think the clocks and resets required by the sequence
> should be provided by the driver. For one, there can be several clocks and
> resets that need to be controlled for a single domain.

Do you have any suggestions for what the API should look like? Even if
we plan to move to some different API, I think there's some advantage in
using it consistently if for no other reason than to make it easier to
replace occurrences later on.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140617/80ed063c/attachment.sig>

[PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Peter De Schrijver]

On Tue, Jun 17, 2014 at 11:51:20PM +0200, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Jun 17, 2014 at 05:01:46PM +0300, Peter De Schrijver wrote:
> > On Tue, Jun 17, 2014 at 02:13:15PM +0200, Thierry Reding wrote:
> > > > Old Signed by an unknown key
> > > 
> > > On Mon, Jun 16, 2014 at 04:01:02PM -0600, Stephen Warren wrote:
> > > > On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> > > > > This symbol needs to be exported to power on rails without using
> > > > > tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
> > > > > cannot be used in situations where the driver wants to handle clocking
> > > > > by itself.
> > > > 
> > > > Thierry, are you OK with this change?
> > > 
> > > I would've preferred tegra_powergate_sequence_power_up() to be used
> > 
> > I don't think the current tegra_powergate_sequence_power_up() API is very well
> > defined though. I don't think the clocks and resets required by the sequence
> > should be provided by the driver. For one, there can be several clocks and
> > resets that need to be controlled for a single domain.
> 
> Do you have any suggestions for what the API should look like? Even if
> we plan to move to some different API, I think there's some advantage in
> using it consistently if for no other reason than to make it easier to
> replace occurrences later on.
> 

I think the API should only have the domain ID as input so:

int tegra_powerdomain_on(int id) 

/*
 * Prerequisites: domain is off
 * Result: domain is on, clocks of the modules in the domain are off, modules are in reset
 */

int tegra_powerdomain_off(int id)

/*
 * Prerequisites: all clocks of the modules in the domain are off
 * result: domain is off
 */

Cheers,

Peter/

[PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Stephen Warren]

On 06/18/2014 06:18 AM, Peter De Schrijver wrote:
> On Tue, Jun 17, 2014 at 11:51:20PM +0200, Thierry Reding wrote:
>> * PGP Signed by an unknown key
>>
>> On Tue, Jun 17, 2014 at 05:01:46PM +0300, Peter De Schrijver wrote:
>>> On Tue, Jun 17, 2014 at 02:13:15PM +0200, Thierry Reding wrote:
>>>>> Old Signed by an unknown key
>>>>
>>>> On Mon, Jun 16, 2014 at 04:01:02PM -0600, Stephen Warren wrote:
>>>>> On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
>>>>>> This symbol needs to be exported to power on rails without using
>>>>>> tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
>>>>>> cannot be used in situations where the driver wants to handle clocking
>>>>>> by itself.
>>>>>
>>>>> Thierry, are you OK with this change?
>>>>
>>>> I would've preferred tegra_powergate_sequence_power_up() to be used
>>>
>>> I don't think the current tegra_powergate_sequence_power_up() API is very well
>>> defined though. I don't think the clocks and resets required by the sequence
>>> should be provided by the driver. For one, there can be several clocks and
>>> resets that need to be controlled for a single domain.
>>
>> Do you have any suggestions for what the API should look like? Even if
>> we plan to move to some different API, I think there's some advantage in
>> using it consistently if for no other reason than to make it easier to
>> replace occurrences later on.
>>
> 
> I think the API should only have the domain ID as input so:
> 
> int tegra_powerdomain_on(int id) 
> 
> /*
>  * Prerequisites: domain is off
>  * Result: domain is on, clocks of the modules in the domain are off, modules are in reset
>  */
> 
> int tegra_powerdomain_off(int id)
> 
> /*
>  * Prerequisites: all clocks of the modules in the domain are off
>  * result: domain is off
>  */

That doesn't make sense; the PMC doesn't have access to the clock and
reset IDs - that's why the API requires them to be passed in.

[PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Peter De Schrijver]

On Wed, Jun 18, 2014 at 05:37:54PM +0200, Stephen Warren wrote:
> On 06/18/2014 06:18 AM, Peter De Schrijver wrote:
> > On Tue, Jun 17, 2014 at 11:51:20PM +0200, Thierry Reding wrote:
> >> * PGP Signed by an unknown key
> >>
> >> On Tue, Jun 17, 2014 at 05:01:46PM +0300, Peter De Schrijver wrote:
> >>> On Tue, Jun 17, 2014 at 02:13:15PM +0200, Thierry Reding wrote:
> >>>>> Old Signed by an unknown key
> >>>>
> >>>> On Mon, Jun 16, 2014 at 04:01:02PM -0600, Stephen Warren wrote:
> >>>>> On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> >>>>>> This symbol needs to be exported to power on rails without using
> >>>>>> tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
> >>>>>> cannot be used in situations where the driver wants to handle clocking
> >>>>>> by itself.
> >>>>>
> >>>>> Thierry, are you OK with this change?
> >>>>
> >>>> I would've preferred tegra_powergate_sequence_power_up() to be used
> >>>
> >>> I don't think the current tegra_powergate_sequence_power_up() API is very well
> >>> defined though. I don't think the clocks and resets required by the sequence
> >>> should be provided by the driver. For one, there can be several clocks and
> >>> resets that need to be controlled for a single domain.
> >>
> >> Do you have any suggestions for what the API should look like? Even if
> >> we plan to move to some different API, I think there's some advantage in
> >> using it consistently if for no other reason than to make it easier to
> >> replace occurrences later on.
> >>
> > 
> > I think the API should only have the domain ID as input so:
> > 
> > int tegra_powerdomain_on(int id) 
> > 
> > /*
> >  * Prerequisites: domain is off
> >  * Result: domain is on, clocks of the modules in the domain are off, modules are in reset
> >  */
> > 
> > int tegra_powerdomain_off(int id)
> > 
> > /*
> >  * Prerequisites: all clocks of the modules in the domain are off
> >  * result: domain is off
> >  */
> 
> That doesn't make sense; the PMC doesn't have access to the clock and
> reset IDs - that's why the API requires them to be passed in.
> 

We should make driver look them up by name then. It doesn't make sense to
move this knowledge to the drivers as there can be several modules in 1
powerdomain. So every driver would then need to have access to all clock
and reset IDs of the modules of the domain?

Cheers,

Peter.

[PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Peter De Schrijver]

On Thu, Jun 19, 2014 at 10:02:35AM +0200, Peter De Schrijver wrote:
> On Wed, Jun 18, 2014 at 05:37:54PM +0200, Stephen Warren wrote:
> > On 06/18/2014 06:18 AM, Peter De Schrijver wrote:
> > > On Tue, Jun 17, 2014 at 11:51:20PM +0200, Thierry Reding wrote:
> > >> * PGP Signed by an unknown key
> > >>
> > >> On Tue, Jun 17, 2014 at 05:01:46PM +0300, Peter De Schrijver wrote:
> > >>> On Tue, Jun 17, 2014 at 02:13:15PM +0200, Thierry Reding wrote:
> > >>>>> Old Signed by an unknown key
> > >>>>
> > >>>> On Mon, Jun 16, 2014 at 04:01:02PM -0600, Stephen Warren wrote:
> > >>>>> On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> > >>>>>> This symbol needs to be exported to power on rails without using
> > >>>>>> tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
> > >>>>>> cannot be used in situations where the driver wants to handle clocking
> > >>>>>> by itself.
> > >>>>>
> > >>>>> Thierry, are you OK with this change?
> > >>>>
> > >>>> I would've preferred tegra_powergate_sequence_power_up() to be used
> > >>>
> > >>> I don't think the current tegra_powergate_sequence_power_up() API is very well
> > >>> defined though. I don't think the clocks and resets required by the sequence
> > >>> should be provided by the driver. For one, there can be several clocks and
> > >>> resets that need to be controlled for a single domain.
> > >>
> > >> Do you have any suggestions for what the API should look like? Even if
> > >> we plan to move to some different API, I think there's some advantage in
> > >> using it consistently if for no other reason than to make it easier to
> > >> replace occurrences later on.
> > >>
> > > 
> > > I think the API should only have the domain ID as input so:
> > > 
> > > int tegra_powerdomain_on(int id) 
> > > 
> > > /*
> > >  * Prerequisites: domain is off
> > >  * Result: domain is on, clocks of the modules in the domain are off, modules are in reset
> > >  */
> > > 
> > > int tegra_powerdomain_off(int id)
> > > 
> > > /*
> > >  * Prerequisites: all clocks of the modules in the domain are off
> > >  * result: domain is off
> > >  */
> > 
> > That doesn't make sense; the PMC doesn't have access to the clock and
> > reset IDs - that's why the API requires them to be passed in.
> > 
> 
> We should make driver look them up by name then. It doesn't make sense to

I mean the PMC driver here.

[PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Stephen Warren]

On 06/19/2014 02:02 AM, Peter De Schrijver wrote:
> On Wed, Jun 18, 2014 at 05:37:54PM +0200, Stephen Warren wrote:
>> On 06/18/2014 06:18 AM, Peter De Schrijver wrote:
>>> On Tue, Jun 17, 2014 at 11:51:20PM +0200, Thierry Reding wrote:
>>>> * PGP Signed by an unknown key
>>>>
>>>> On Tue, Jun 17, 2014 at 05:01:46PM +0300, Peter De Schrijver wrote:
>>>>> On Tue, Jun 17, 2014 at 02:13:15PM +0200, Thierry Reding wrote:
>>>>>>> Old Signed by an unknown key
>>>>>>
>>>>>> On Mon, Jun 16, 2014 at 04:01:02PM -0600, Stephen Warren wrote:
>>>>>>> On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
>>>>>>>> This symbol needs to be exported to power on rails without using
>>>>>>>> tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
>>>>>>>> cannot be used in situations where the driver wants to handle clocking
>>>>>>>> by itself.
>>>>>>>
>>>>>>> Thierry, are you OK with this change?
>>>>>>
>>>>>> I would've preferred tegra_powergate_sequence_power_up() to be used
>>>>>
>>>>> I don't think the current tegra_powergate_sequence_power_up() API is very well
>>>>> defined though. I don't think the clocks and resets required by the sequence
>>>>> should be provided by the driver. For one, there can be several clocks and
>>>>> resets that need to be controlled for a single domain.
>>>>
>>>> Do you have any suggestions for what the API should look like? Even if
>>>> we plan to move to some different API, I think there's some advantage in
>>>> using it consistently if for no other reason than to make it easier to
>>>> replace occurrences later on.
>>>>
>>>
>>> I think the API should only have the domain ID as input so:
>>>
>>> int tegra_powerdomain_on(int id) 
>>>
>>> /*
>>>  * Prerequisites: domain is off
>>>  * Result: domain is on, clocks of the modules in the domain are off, modules are in reset
>>>  */
>>>
>>> int tegra_powerdomain_off(int id)
>>>
>>> /*
>>>  * Prerequisites: all clocks of the modules in the domain are off
>>>  * result: domain is off
>>>  */
>>
>> That doesn't make sense; the PMC doesn't have access to the clock and
>> reset IDs - that's why the API requires them to be passed in.
>>
> 
> We should make driver look them up by name then. It doesn't make sense to
> move this knowledge to the drivers as there can be several modules in 1
> powerdomain. So every driver would then need to have access to all clock
> and reset IDs of the modules of the domain?

Having the drivers know the clocks, resets, and power domains that
affect their HW module seems perfectly reasonable.

Do we actually have any case where unrelated modules are in the same
power domain /and/ there's some need for those drivers to manipulate the
power domain?

BTW, any objections here should have been raised when the initial API
design for powergating was created. Now that we have the current API,
which in turn has driven the DT bindings for at least some HW modules,
we're stuck with it due to DT ABIness. Well, perhaps we can introduce
something new, but we'll always have to support the old way, so there's
little point.

[PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Peter De Schrijver]

On Thu, Jun 19, 2014 at 06:01:47PM +0200, Stephen Warren wrote:
> On 06/19/2014 02:02 AM, Peter De Schrijver wrote:
> > On Wed, Jun 18, 2014 at 05:37:54PM +0200, Stephen Warren wrote:
> >> On 06/18/2014 06:18 AM, Peter De Schrijver wrote:
> >>> On Tue, Jun 17, 2014 at 11:51:20PM +0200, Thierry Reding wrote:
> >>>> * PGP Signed by an unknown key
> >>>>
> >>>> On Tue, Jun 17, 2014 at 05:01:46PM +0300, Peter De Schrijver wrote:
> >>>>> On Tue, Jun 17, 2014 at 02:13:15PM +0200, Thierry Reding wrote:
> >>>>>>> Old Signed by an unknown key
> >>>>>>
> >>>>>> On Mon, Jun 16, 2014 at 04:01:02PM -0600, Stephen Warren wrote:
> >>>>>>> On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> >>>>>>>> This symbol needs to be exported to power on rails without using
> >>>>>>>> tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
> >>>>>>>> cannot be used in situations where the driver wants to handle clocking
> >>>>>>>> by itself.
> >>>>>>>
> >>>>>>> Thierry, are you OK with this change?
> >>>>>>
> >>>>>> I would've preferred tegra_powergate_sequence_power_up() to be used
> >>>>>
> >>>>> I don't think the current tegra_powergate_sequence_power_up() API is very well
> >>>>> defined though. I don't think the clocks and resets required by the sequence
> >>>>> should be provided by the driver. For one, there can be several clocks and
> >>>>> resets that need to be controlled for a single domain.
> >>>>
> >>>> Do you have any suggestions for what the API should look like? Even if
> >>>> we plan to move to some different API, I think there's some advantage in
> >>>> using it consistently if for no other reason than to make it easier to
> >>>> replace occurrences later on.
> >>>>
> >>>
> >>> I think the API should only have the domain ID as input so:
> >>>
> >>> int tegra_powerdomain_on(int id) 
> >>>
> >>> /*
> >>>  * Prerequisites: domain is off
> >>>  * Result: domain is on, clocks of the modules in the domain are off, modules are in reset
> >>>  */
> >>>
> >>> int tegra_powerdomain_off(int id)
> >>>
> >>> /*
> >>>  * Prerequisites: all clocks of the modules in the domain are off
> >>>  * result: domain is off
> >>>  */
> >>
> >> That doesn't make sense; the PMC doesn't have access to the clock and
> >> reset IDs - that's why the API requires them to be passed in.
> >>
> > 
> > We should make driver look them up by name then. It doesn't make sense to
> > move this knowledge to the drivers as there can be several modules in 1
> > powerdomain. So every driver would then need to have access to all clock
> > and reset IDs of the modules of the domain?
> 
> Having the drivers know the clocks, resets, and power domains that
> affect their HW module seems perfectly reasonable.
> 

Yes, but the problem is that you also need clocks and reset of other modules
in the same domain to safely control the domain's status. Eg: the ISPs, VI and
CSI share a domain. VI and CSI are useable without ISP and the ISP lacks
public documentation. So it's not unlikely a VI and CSI driver will upstreamed
someday which means we would need to control the domain and therefore would
need to tell that driver about the ISPs clocks and resets even though the
driver doesn't know anything about the ISP hw otherwise.

> Do we actually have any case where unrelated modules are in the same
> power domain /and/ there's some need for those drivers to manipulate the
> power domain?
> 

According to the TRM, the reset state of the power domains is undefined. While
that seems unlikely, I do think the kernel should not assume any domain is on
apart from the obvious ones (like the CPU domain it is running on),

> BTW, any objections here should have been raised when the initial API
> design for powergating was created. Now that we have the current API,
> which in turn has driven the DT bindings for at least some HW modules,
> we're stuck with it due to DT ABIness. Well, perhaps we can introduce
> something new, but we'll always have to support the old way, so there's
> little point.

I don't think the bindings need to be changed at all as long as the PMC driver
can parse the relevant DT nodes to get the clock and reset information.

Cheers,

Peter.

[PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Thierry Reding]

On Mon, Jun 23, 2014 at 01:14:42PM +0300, Peter De Schrijver wrote:
> On Thu, Jun 19, 2014 at 06:01:47PM +0200, Stephen Warren wrote:
> > On 06/19/2014 02:02 AM, Peter De Schrijver wrote:
> > > On Wed, Jun 18, 2014 at 05:37:54PM +0200, Stephen Warren wrote:
> > >> On 06/18/2014 06:18 AM, Peter De Schrijver wrote:
> > >>> On Tue, Jun 17, 2014 at 11:51:20PM +0200, Thierry Reding wrote:
> > >>>> * PGP Signed by an unknown key
> > >>>>
> > >>>> On Tue, Jun 17, 2014 at 05:01:46PM +0300, Peter De Schrijver wrote:
> > >>>>> On Tue, Jun 17, 2014 at 02:13:15PM +0200, Thierry Reding wrote:
> > >>>>>>> Old Signed by an unknown key
> > >>>>>>
> > >>>>>> On Mon, Jun 16, 2014 at 04:01:02PM -0600, Stephen Warren wrote:
> > >>>>>>> On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> > >>>>>>>> This symbol needs to be exported to power on rails without using
> > >>>>>>>> tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
> > >>>>>>>> cannot be used in situations where the driver wants to handle clocking
> > >>>>>>>> by itself.
> > >>>>>>>
> > >>>>>>> Thierry, are you OK with this change?
> > >>>>>>
> > >>>>>> I would've preferred tegra_powergate_sequence_power_up() to be used
> > >>>>>
> > >>>>> I don't think the current tegra_powergate_sequence_power_up() API is very well
> > >>>>> defined though. I don't think the clocks and resets required by the sequence
> > >>>>> should be provided by the driver. For one, there can be several clocks and
> > >>>>> resets that need to be controlled for a single domain.
> > >>>>
> > >>>> Do you have any suggestions for what the API should look like? Even if
> > >>>> we plan to move to some different API, I think there's some advantage in
> > >>>> using it consistently if for no other reason than to make it easier to
> > >>>> replace occurrences later on.
> > >>>>
> > >>>
> > >>> I think the API should only have the domain ID as input so:
> > >>>
> > >>> int tegra_powerdomain_on(int id) 
> > >>>
> > >>> /*
> > >>>  * Prerequisites: domain is off
> > >>>  * Result: domain is on, clocks of the modules in the domain are off, modules are in reset
> > >>>  */
> > >>>
> > >>> int tegra_powerdomain_off(int id)
> > >>>
> > >>> /*
> > >>>  * Prerequisites: all clocks of the modules in the domain are off
> > >>>  * result: domain is off
> > >>>  */
> > >>
> > >> That doesn't make sense; the PMC doesn't have access to the clock and
> > >> reset IDs - that's why the API requires them to be passed in.
> > >>
> > > 
> > > We should make driver look them up by name then. It doesn't make sense to
> > > move this knowledge to the drivers as there can be several modules in 1
> > > powerdomain. So every driver would then need to have access to all clock
> > > and reset IDs of the modules of the domain?
> > 
> > Having the drivers know the clocks, resets, and power domains that
> > affect their HW module seems perfectly reasonable.
> > 
> 
> Yes, but the problem is that you also need clocks and reset of other modules
> in the same domain to safely control the domain's status. Eg: the ISPs, VI and
> CSI share a domain. VI and CSI are useable without ISP and the ISP lacks
> public documentation. So it's not unlikely a VI and CSI driver will upstreamed
> someday which means we would need to control the domain and therefore would
> need to tell that driver about the ISPs clocks and resets even though the
> driver doesn't know anything about the ISP hw otherwise.

Can't we make powergates reference counted so that they don't get
disabled as long as there are any users? Looking for example at the
display controller driver, modules don't seem to care overly much about
the powergate's state except that it needs to be turned on before they
touch some of the registers.

>From a bit of experimentation it also seems like the sequence encoded
within tegra_powergate_sequence_power_up() isn't at all necessary. I
couldn't find an authoritative reference for that either, so I'm tempted
to conclude that it was simply cargo-culted from the dark-ages.

So I'm thinking that if we ever move to use power domains for this, we
may be able to just drop any extra handling (well, we'd need to keep it
for backwards-compatibility... *sigh*) and let drivers handle the clock
and reset resources.

On the other hand, given that we already need to keep the existing code
for backwards-compatibility, I'm not sure there's a real advantage in
turning them into power domains, since we'd be adding extra code without
an clear gains (especially since it seems like we'd need even more code
to properly handle suspend/resume in drivers that need powergates).

> > Do we actually have any case where unrelated modules are in the same
> > power domain /and/ there's some need for those drivers to manipulate the
> > power domain?
> > 
> 
> According to the TRM, the reset state of the power domains is undefined. While
> that seems unlikely, I do think the kernel should not assume any domain is on
> apart from the obvious ones (like the CPU domain it is running on),

Hm... I thought I had seen some documents specifically giving the reset
states of the power partitions. Perhaps it wasn't in the TRM, though.
But I agree that drivers generally shouldn't be assuming anything about
the power partition states.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140708/c9da3d66/attachment.sig>

[PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Peter De Schrijver]

> > 
> > Yes, but the problem is that you also need clocks and reset of other modules
> > in the same domain to safely control the domain's status. Eg: the ISPs, VI and
> > CSI share a domain. VI and CSI are useable without ISP and the ISP lacks
> > public documentation. So it's not unlikely a VI and CSI driver will upstreamed
> > someday which means we would need to control the domain and therefore would
> > need to tell that driver about the ISPs clocks and resets even though the
> > driver doesn't know anything about the ISP hw otherwise.
> 
> Can't we make powergates reference counted so that they don't get
> disabled as long as there are any users? Looking for example at the

We could, but then why not switch to the powerdomain code and make powering
off a domain a NOP until we sorted out the context save/restore or fixed
the framework to allow for suspend without turning off the domains?

> display controller driver, modules don't seem to care overly much about
> the powergate's state except that it needs to be turned on before they
> touch some of the registers.
> 
> From a bit of experimentation it also seems like the sequence encoded
> within tegra_powergate_sequence_power_up() isn't at all necessary. I
> couldn't find an authoritative reference for that either, so I'm tempted
> to conclude that it was simply cargo-culted from the dark-ages.
> 
> So I'm thinking that if we ever move to use power domains for this, we
> may be able to just drop any extra handling (well, we'd need to keep it
> for backwards-compatibility... *sigh*) and let drivers handle the clock
> and reset resources.
> 
> On the other hand, given that we already need to keep the existing code
> for backwards-compatibility, I'm not sure there's a real advantage in
> turning them into power domains, since we'd be adding extra code without
> an clear gains (especially since it seems like we'd need even more code
> to properly handle suspend/resume in drivers that need powergates).
> 

Unless we fix the framework to require context save/restore for suspend.
There is a good reason to do that. context save/restore requires energy
as well, so it's not a given that turning off domains in system suspend
will save power.

Cheers,

Peter.

[PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Thierry Reding]

On Tue, Jul 08, 2014 at 05:11:35PM +0300, Peter De Schrijver wrote:
> > > 
> > > Yes, but the problem is that you also need clocks and reset of other modules
> > > in the same domain to safely control the domain's status. Eg: the ISPs, VI and
> > > CSI share a domain. VI and CSI are useable without ISP and the ISP lacks
> > > public documentation. So it's not unlikely a VI and CSI driver will upstreamed
> > > someday which means we would need to control the domain and therefore would
> > > need to tell that driver about the ISPs clocks and resets even though the
> > > driver doesn't know anything about the ISP hw otherwise.
> > 
> > Can't we make powergates reference counted so that they don't get
> > disabled as long as there are any users? Looking for example at the
> 
> We could, but then why not switch to the powerdomain code and make powering
> off a domain a NOP until we sorted out the context save/restore or fixed
> the framework to allow for suspend without turning off the domains?

Well, one of the reasons why I'm not sure it's worth the effort at this
point is that we can't get rid of the tegra_powergate_*() API anyway
because of backwards compatibility. So we're going to add code (without
getting rid of old code) merely to support some generic framework. That
doesn't sound very useful to me.

> > display controller driver, modules don't seem to care overly much about
> > the powergate's state except that it needs to be turned on before they
> > touch some of the registers.
> > 
> > From a bit of experimentation it also seems like the sequence encoded
> > within tegra_powergate_sequence_power_up() isn't at all necessary. I
> > couldn't find an authoritative reference for that either, so I'm tempted
> > to conclude that it was simply cargo-culted from the dark-ages.
> > 
> > So I'm thinking that if we ever move to use power domains for this, we
> > may be able to just drop any extra handling (well, we'd need to keep it
> > for backwards-compatibility... *sigh*) and let drivers handle the clock
> > and reset resources.
> > 
> > On the other hand, given that we already need to keep the existing code
> > for backwards-compatibility, I'm not sure there's a real advantage in
> > turning them into power domains, since we'd be adding extra code without
> > an clear gains (especially since it seems like we'd need even more code
> > to properly handle suspend/resume in drivers that need powergates).
> > 
> 
> Unless we fix the framework to require context save/restore for suspend.
> There is a good reason to do that. context save/restore requires energy
> as well, so it's not a given that turning off domains in system suspend
> will save power.

I'm not sure I follow. "require context save/restore for suspend" is
what many drivers currently don't support, hence we can't use the
generic PM domains. Perhaps what you're saying is that the PM domain
core code should be enhanced so that domains can be marked so that they
stay on during a suspend/resume cycle.

Such functionality could be part of a specifier in a DT binding. It's
technically not a description of the hardware, but it encodes a device's
requirements regarding suspend/resume latency.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140709/c4b2c4ed/attachment.sig>

[PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Peter De Schrijver]

On Wed, Jul 09, 2014 at 08:31:32AM +0200, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Jul 08, 2014 at 05:11:35PM +0300, Peter De Schrijver wrote:
> > > > 
> > > > Yes, but the problem is that you also need clocks and reset of other modules
> > > > in the same domain to safely control the domain's status. Eg: the ISPs, VI and
> > > > CSI share a domain. VI and CSI are useable without ISP and the ISP lacks
> > > > public documentation. So it's not unlikely a VI and CSI driver will upstreamed
> > > > someday which means we would need to control the domain and therefore would
> > > > need to tell that driver about the ISPs clocks and resets even though the
> > > > driver doesn't know anything about the ISP hw otherwise.
> > > 
> > > Can't we make powergates reference counted so that they don't get
> > > disabled as long as there are any users? Looking for example at the
> > 
> > We could, but then why not switch to the powerdomain code and make powering
> > off a domain a NOP until we sorted out the context save/restore or fixed
> > the framework to allow for suspend without turning off the domains?
> 
> Well, one of the reasons why I'm not sure it's worth the effort at this
> point is that we can't get rid of the tegra_powergate_*() API anyway
> because of backwards compatibility. So we're going to add code (without
> getting rid of old code) merely to support some generic framework. That
> doesn't sound very useful to me.
> 

We can also convert the existing users to genpd. Today there are only 2 users
(gpu/drm/tegra/gr3d.c and pci/host/pci-tegra.c), so that doesn't seem to be
an impossible task.

> > > display controller driver, modules don't seem to care overly much about
> > > the powergate's state except that it needs to be turned on before they
> > > touch some of the registers.
> > > 
> > > From a bit of experimentation it also seems like the sequence encoded
> > > within tegra_powergate_sequence_power_up() isn't at all necessary. I
> > > couldn't find an authoritative reference for that either, so I'm tempted
> > > to conclude that it was simply cargo-culted from the dark-ages.
> > > 
> > > So I'm thinking that if we ever move to use power domains for this, we
> > > may be able to just drop any extra handling (well, we'd need to keep it
> > > for backwards-compatibility... *sigh*) and let drivers handle the clock
> > > and reset resources.
> > > 
> > > On the other hand, given that we already need to keep the existing code
> > > for backwards-compatibility, I'm not sure there's a real advantage in
> > > turning them into power domains, since we'd be adding extra code without
> > > an clear gains (especially since it seems like we'd need even more code
> > > to properly handle suspend/resume in drivers that need powergates).
> > > 
> > 
> > Unless we fix the framework to require context save/restore for suspend.
> > There is a good reason to do that. context save/restore requires energy
> > as well, so it's not a given that turning off domains in system suspend
> > will save power.
> 
> I'm not sure I follow. "require context save/restore for suspend" is
> what many drivers currently don't support, hence we can't use the
> generic PM domains. Perhaps what you're saying is that the PM domain
> core code should be enhanced so that domains can be marked so that they
> stay on during a suspend/resume cycle.
> 

Exactly.

Cheers,

Peter.

[PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Thierry Reding]

On Wed, Jul 09, 2014 at 11:33:11AM +0300, Peter De Schrijver wrote:
> On Wed, Jul 09, 2014 at 08:31:32AM +0200, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Tue, Jul 08, 2014 at 05:11:35PM +0300, Peter De Schrijver wrote:
> > > > > 
> > > > > Yes, but the problem is that you also need clocks and reset of other modules
> > > > > in the same domain to safely control the domain's status. Eg: the ISPs, VI and
> > > > > CSI share a domain. VI and CSI are useable without ISP and the ISP lacks
> > > > > public documentation. So it's not unlikely a VI and CSI driver will upstreamed
> > > > > someday which means we would need to control the domain and therefore would
> > > > > need to tell that driver about the ISPs clocks and resets even though the
> > > > > driver doesn't know anything about the ISP hw otherwise.
> > > > 
> > > > Can't we make powergates reference counted so that they don't get
> > > > disabled as long as there are any users? Looking for example at the
> > > 
> > > We could, but then why not switch to the powerdomain code and make powering
> > > off a domain a NOP until we sorted out the context save/restore or fixed
> > > the framework to allow for suspend without turning off the domains?
> > 
> > Well, one of the reasons why I'm not sure it's worth the effort at this
> > point is that we can't get rid of the tegra_powergate_*() API anyway
> > because of backwards compatibility. So we're going to add code (without
> > getting rid of old code) merely to support some generic framework. That
> > doesn't sound very useful to me.
> > 
> 
> We can also convert the existing users to genpd. Today there are only 2 users
> (gpu/drm/tegra/gr3d.c and pci/host/pci-tegra.c), so that doesn't seem to be
> an impossible task.

We can certainly do that. What I'm trying to say is that since people
may be running newer versions of the kernel with a DTB that doesn't have
the necessary properties to hook up power domains, we have to keep calls
to tegra_powergate_*() functions as-is, lest we break those setups.

That then means that each such driver needs a way to figure out whether
power domains are hooked up (which will be rather clumsy to do if it's
all supposed to work transparently) and fallback to the legacy API
otherwise.

So essentially we'll be adding code without the possibility to remove
any of the old code.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140709/26e9c5cc/attachment.sig>

[PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Lucas Stach]

Am Mittwoch, den 09.07.2014, 12:25 +0200 schrieb Thierry Reding:
> On Wed, Jul 09, 2014 at 11:33:11AM +0300, Peter De Schrijver wrote:
> > On Wed, Jul 09, 2014 at 08:31:32AM +0200, Thierry Reding wrote:
> > > * PGP Signed by an unknown key
> > > 
> > > On Tue, Jul 08, 2014 at 05:11:35PM +0300, Peter De Schrijver wrote:
> > > > > > 
> > > > > > Yes, but the problem is that you also need clocks and reset of other modules
> > > > > > in the same domain to safely control the domain's status. Eg: the ISPs, VI and
> > > > > > CSI share a domain. VI and CSI are useable without ISP and the ISP lacks
> > > > > > public documentation. So it's not unlikely a VI and CSI driver will upstreamed
> > > > > > someday which means we would need to control the domain and therefore would
> > > > > > need to tell that driver about the ISPs clocks and resets even though the
> > > > > > driver doesn't know anything about the ISP hw otherwise.
> > > > > 
> > > > > Can't we make powergates reference counted so that they don't get
> > > > > disabled as long as there are any users? Looking for example at the
> > > > 
> > > > We could, but then why not switch to the powerdomain code and make powering
> > > > off a domain a NOP until we sorted out the context save/restore or fixed
> > > > the framework to allow for suspend without turning off the domains?
> > > 
> > > Well, one of the reasons why I'm not sure it's worth the effort at this
> > > point is that we can't get rid of the tegra_powergate_*() API anyway
> > > because of backwards compatibility. So we're going to add code (without
> > > getting rid of old code) merely to support some generic framework. That
> > > doesn't sound very useful to me.
> > > 
> > 
> > We can also convert the existing users to genpd. Today there are only 2 users
> > (gpu/drm/tegra/gr3d.c and pci/host/pci-tegra.c), so that doesn't seem to be
> > an impossible task.
> 
> We can certainly do that. What I'm trying to say is that since people
> may be running newer versions of the kernel with a DTB that doesn't have
> the necessary properties to hook up power domains, we have to keep calls
> to tegra_powergate_*() functions as-is, lest we break those setups.
> 
> That then means that each such driver needs a way to figure out whether
> power domains are hooked up (which will be rather clumsy to do if it's
> all supposed to work transparently) and fallback to the legacy API
> otherwise.
> 
> So essentially we'll be adding code without the possibility to remove
> any of the old code.
> 
You could also take the easy way out and enable all power domains by
default. So drivers won't be able to disable the power domain on old
DTBs where they aren't hooked up, but the will continue to function
without calling into the old Tegra specific power domain code.

Regards,
Lucas
-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

[PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Peter De Schrijver]

On Wed, Jul 09, 2014 at 12:31:47PM +0200, Lucas Stach wrote:
> > 
> You could also take the easy way out and enable all power domains by
> default. So drivers won't be able to disable the power domain on old
> DTBs where they aren't hooked up, but the will continue to function
> without calling into the old Tegra specific power domain code.
> 

If the bootloader turned some domains off, the kernel would have to turn them
back on again. However that requires knowledge about the clocks and resets of
the peripherals in the domain.

Cheers,

Peter.

[PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Peter De Schrijver]

On Wed, Jul 09, 2014 at 12:25:52PM +0200, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Wed, Jul 09, 2014 at 11:33:11AM +0300, Peter De Schrijver wrote:
> > On Wed, Jul 09, 2014 at 08:31:32AM +0200, Thierry Reding wrote:
> > > > Old Signed by an unknown key
> > > 
> > > On Tue, Jul 08, 2014 at 05:11:35PM +0300, Peter De Schrijver wrote:
> > > > > > 
> > > > > > Yes, but the problem is that you also need clocks and reset of other modules
> > > > > > in the same domain to safely control the domain's status. Eg: the ISPs, VI and
> > > > > > CSI share a domain. VI and CSI are useable without ISP and the ISP lacks
> > > > > > public documentation. So it's not unlikely a VI and CSI driver will upstreamed
> > > > > > someday which means we would need to control the domain and therefore would
> > > > > > need to tell that driver about the ISPs clocks and resets even though the
> > > > > > driver doesn't know anything about the ISP hw otherwise.
> > > > > 
> > > > > Can't we make powergates reference counted so that they don't get
> > > > > disabled as long as there are any users? Looking for example at the
> > > > 
> > > > We could, but then why not switch to the powerdomain code and make powering
> > > > off a domain a NOP until we sorted out the context save/restore or fixed
> > > > the framework to allow for suspend without turning off the domains?
> > > 
> > > Well, one of the reasons why I'm not sure it's worth the effort at this
> > > point is that we can't get rid of the tegra_powergate_*() API anyway
> > > because of backwards compatibility. So we're going to add code (without
> > > getting rid of old code) merely to support some generic framework. That
> > > doesn't sound very useful to me.
> > > 
> > 
> > We can also convert the existing users to genpd. Today there are only 2 users
> > (gpu/drm/tegra/gr3d.c and pci/host/pci-tegra.c), so that doesn't seem to be
> > an impossible task.
> 
> We can certainly do that. What I'm trying to say is that since people
> may be running newer versions of the kernel with a DTB that doesn't have
> the necessary properties to hook up power domains, we have to keep calls
> to tegra_powergate_*() functions as-is, lest we break those setups.
> 

For those 2 domains we can find the necessary clocks and resets by parsing
the relevant existing DT nodes for PCIe and gr3d. For clocks, this isn't
even needed as we can always register some extra clkdev's to get them. There
is no equivalent for resets so we have to parse the gr3d and pcie DT nodes,
but that's not too bad I think.

Cheers,

Peter.

[PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Thierry Reding]

On Wed, Jul 09, 2014 at 02:08:16PM +0300, Peter De Schrijver wrote:
> On Wed, Jul 09, 2014 at 12:25:52PM +0200, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Wed, Jul 09, 2014 at 11:33:11AM +0300, Peter De Schrijver wrote:
> > > On Wed, Jul 09, 2014 at 08:31:32AM +0200, Thierry Reding wrote:
> > > > > Old Signed by an unknown key
> > > > 
> > > > On Tue, Jul 08, 2014 at 05:11:35PM +0300, Peter De Schrijver wrote:
> > > > > > > 
> > > > > > > Yes, but the problem is that you also need clocks and reset of other modules
> > > > > > > in the same domain to safely control the domain's status. Eg: the ISPs, VI and
> > > > > > > CSI share a domain. VI and CSI are useable without ISP and the ISP lacks
> > > > > > > public documentation. So it's not unlikely a VI and CSI driver will upstreamed
> > > > > > > someday which means we would need to control the domain and therefore would
> > > > > > > need to tell that driver about the ISPs clocks and resets even though the
> > > > > > > driver doesn't know anything about the ISP hw otherwise.
> > > > > > 
> > > > > > Can't we make powergates reference counted so that they don't get
> > > > > > disabled as long as there are any users? Looking for example at the
> > > > > 
> > > > > We could, but then why not switch to the powerdomain code and make powering
> > > > > off a domain a NOP until we sorted out the context save/restore or fixed
> > > > > the framework to allow for suspend without turning off the domains?
> > > > 
> > > > Well, one of the reasons why I'm not sure it's worth the effort at this
> > > > point is that we can't get rid of the tegra_powergate_*() API anyway
> > > > because of backwards compatibility. So we're going to add code (without
> > > > getting rid of old code) merely to support some generic framework. That
> > > > doesn't sound very useful to me.
> > > > 
> > > 
> > > We can also convert the existing users to genpd. Today there are only 2 users
> > > (gpu/drm/tegra/gr3d.c and pci/host/pci-tegra.c), so that doesn't seem to be
> > > an impossible task.
> > 
> > We can certainly do that. What I'm trying to say is that since people
> > may be running newer versions of the kernel with a DTB that doesn't have
> > the necessary properties to hook up power domains, we have to keep calls
> > to tegra_powergate_*() functions as-is, lest we break those setups.
> > 
> 
> For those 2 domains we can find the necessary clocks and resets by parsing
> the relevant existing DT nodes for PCIe and gr3d. For clocks, this isn't
> even needed as we can always register some extra clkdev's to get them. There
> is no equivalent for resets so we have to parse the gr3d and pcie DT nodes,
> but that's not too bad I think.

Even if we could really do this, at this point I don't see an advantage.
All that it would be doing is move to some subsystem that doesn't quite
match what we need just for the sake of moving to that subsystem. Having
a Tegra-specific API doesn't sound so bad anymore.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140709/5d6547f7/attachment.sig>

[PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Peter De Schrijver]

On Wed, Jul 09, 2014 at 02:04:02PM +0200, Thierry Reding wrote:
> > For those 2 domains we can find the necessary clocks and resets by parsing
> > the relevant existing DT nodes for PCIe and gr3d. For clocks, this isn't
> > even needed as we can always register some extra clkdev's to get them. There
> > is no equivalent for resets so we have to parse the gr3d and pcie DT nodes,
> > but that's not too bad I think.
> 
> Even if we could really do this, at this point I don't see an advantage.
> All that it would be doing is move to some subsystem that doesn't quite
> match what we need just for the sake of moving to that subsystem. Having
> a Tegra-specific API doesn't sound so bad anymore.
> 

The advantage would be that we can use LP0/SC7 as a cpuidle state. Also system
resume from LP0 can be faster as we potentially don't have to resume all
domains at once.

Cheers,

Peter.

[PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Thierry Reding]

On Wed, Jul 09, 2014 at 03:43:44PM +0300, Peter De Schrijver wrote:
> On Wed, Jul 09, 2014 at 02:04:02PM +0200, Thierry Reding wrote:
> > > For those 2 domains we can find the necessary clocks and resets by parsing
> > > the relevant existing DT nodes for PCIe and gr3d. For clocks, this isn't
> > > even needed as we can always register some extra clkdev's to get them. There
> > > is no equivalent for resets so we have to parse the gr3d and pcie DT nodes,
> > > but that's not too bad I think.
> > 
> > Even if we could really do this, at this point I don't see an advantage.
> > All that it would be doing is move to some subsystem that doesn't quite
> > match what we need just for the sake of moving to that subsystem. Having
> > a Tegra-specific API doesn't sound so bad anymore.
> > 
> 
> The advantage would be that we can use LP0/SC7 as a cpuidle state.

How is that going to work? And why does it need powergates to be
implemented as power domains? If we switch off power gates, then we need
to restore context in the drivers anyway, therefore I assume .suspend()
and .resume() would need to be called, in which case powergate handling
can surely be done at that stage, can't it?

> Also system
> resume from LP0 can be faster as we potentially don't have to resume all
> domains at once.

I don't understand what that's got to do with anything. If we call into
the PMC driver explicitly via tegra_powergate_*() functions from driver
code, then we have full control over suspend/resume in the drivers, and
therefore don't need to resume all at once either.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140709/0899cb01/attachment.sig>

[PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Peter De Schrijver]

On Wed, Jul 09, 2014 at 02:56:14PM +0200, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Wed, Jul 09, 2014 at 03:43:44PM +0300, Peter De Schrijver wrote:
> > On Wed, Jul 09, 2014 at 02:04:02PM +0200, Thierry Reding wrote:
> > > > For those 2 domains we can find the necessary clocks and resets by parsing
> > > > the relevant existing DT nodes for PCIe and gr3d. For clocks, this isn't
> > > > even needed as we can always register some extra clkdev's to get them. There
> > > > is no equivalent for resets so we have to parse the gr3d and pcie DT nodes,
> > > > but that's not too bad I think.
> > > 
> > > Even if we could really do this, at this point I don't see an advantage.
> > > All that it would be doing is move to some subsystem that doesn't quite
> > > match what we need just for the sake of moving to that subsystem. Having
> > > a Tegra-specific API doesn't sound so bad anymore.
> > > 
> > 
> > The advantage would be that we can use LP0/SC7 as a cpuidle state.
> 
> How is that going to work? And why does it need powergates to be

pm_runtime_get() and pm_runtime_put() hook into genpd. So genpd knows
when all devices in a domain are idle. It can then decide to turn off
the domain (based on the decision of a per domain governor). If all
domains are off (except for the non-powergateable domain), a special cpuidle
state can be enabled by genpd which will initiate a transition to LP0 without
actually doing a full system suspend.

> implemented as power domains? If we switch off power gates, then we need
> to restore context in the drivers anyway, therefore I assume .suspend()
> and .resume() would need to be called, in which case powergate handling
> can surely be done at that stage, can't it?
> 

.suspend() and .resume() are not used for this. genpd uses other per device
callbacks to save and restore the state which are invoked when the domain
is turned off and on (.save_state and .restore_state). The major difference
with .suspend() and .resume() is that .suspend() has to perform 3 tasks:
prevent any new requests to the driver, finalize or cancel all outstanding
requests and save the hw context. .save_state will only be called when the
device is idle (based on the refcount controlled by pm_runtime_get() and
pm_runtime_put()) which means it only has to handle saving the hw context.

> > Also system
> > resume from LP0 can be faster as we potentially don't have to resume all
> > domains at once.
> 
> I don't understand what that's got to do with anything. If we call into
> the PMC driver explicitly via tegra_powergate_*() functions from driver
> code, then we have full control over suspend/resume in the drivers, and
> therefore don't need to resume all at once either.

But then we would be duplicating all the bookkeeping required for this? What's
the point of that?

Cheers,

Peter.

[PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Thierry Reding]

On Wed, Jul 09, 2014 at 04:20:10PM +0300, Peter De Schrijver wrote:
> On Wed, Jul 09, 2014 at 02:56:14PM +0200, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Wed, Jul 09, 2014 at 03:43:44PM +0300, Peter De Schrijver wrote:
> > > On Wed, Jul 09, 2014 at 02:04:02PM +0200, Thierry Reding wrote:
> > > > > For those 2 domains we can find the necessary clocks and resets by parsing
> > > > > the relevant existing DT nodes for PCIe and gr3d. For clocks, this isn't
> > > > > even needed as we can always register some extra clkdev's to get them. There
> > > > > is no equivalent for resets so we have to parse the gr3d and pcie DT nodes,
> > > > > but that's not too bad I think.
> > > > 
> > > > Even if we could really do this, at this point I don't see an advantage.
> > > > All that it would be doing is move to some subsystem that doesn't quite
> > > > match what we need just for the sake of moving to that subsystem. Having
> > > > a Tegra-specific API doesn't sound so bad anymore.
> > > > 
> > > 
> > > The advantage would be that we can use LP0/SC7 as a cpuidle state.
> > 
> > How is that going to work? And why does it need powergates to be
> 
> pm_runtime_get() and pm_runtime_put() hook into genpd. So genpd knows
> when all devices in a domain are idle. It can then decide to turn off
> the domain (based on the decision of a per domain governor). If all
> domains are off (except for the non-powergateable domain), a special cpuidle
> state can be enabled by genpd which will initiate a transition to LP0 without
> actually doing a full system suspend.

Okay, I see.

> > implemented as power domains? If we switch off power gates, then we need
> > to restore context in the drivers anyway, therefore I assume .suspend()
> > and .resume() would need to be called, in which case powergate handling
> > can surely be done at that stage, can't it?
> > 
> 
> .suspend() and .resume() are not used for this. genpd uses other per device
> callbacks to save and restore the state which are invoked when the domain
> is turned off and on (.save_state and .restore_state). The major difference
> with .suspend() and .resume() is that .suspend() has to perform 3 tasks:
> prevent any new requests to the driver, finalize or cancel all outstanding
> requests and save the hw context. .save_state will only be called when the
> device is idle (based on the refcount controlled by pm_runtime_get() and
> pm_runtime_put()) which means it only has to handle saving the hw context.

With the above, would it be possible to make turning off the domain
conditional on whether or not all devices in the domain implement
.save_state() and .restore_state()? That would allow us to convert to
power domains and then stage in context save/restore in drivers (or even
leave it out if there's not enough to be gained from turning the
partition off).

> > > Also system
> > > resume from LP0 can be faster as we potentially don't have to resume all
> > > domains at once.
> > 
> > I don't understand what that's got to do with anything. If we call into
> > the PMC driver explicitly via tegra_powergate_*() functions from driver
> > code, then we have full control over suspend/resume in the drivers, and
> > therefore don't need to resume all at once either.
> 
> But then we would be duplicating all the bookkeeping required for this? What's
> the point of that?

We're doing fine without any bookkeeping currently. I understand that
this may change eventually, but I'm hesitant to start any conversion
like this before we don't have a better understanding of how it should
work (and actual use-cases which we can test). Also we've seen in the
past that coding things up before we have enough use-cases we're bound
to fail at coming up with a proper binding and then we have to keep
carrying loads of code for compatibility.

So if you're willing to give this a shot, I'm not at all opposed to it
generally. But we need to make sure that both the binding is reasonably
future-proof and that we can actually test things like reference-counted
power domains.

Now in the meantime there are a bunch of other drivers that will need to
use the powergate API. DC is one of them. We haven't needed this before
because we assumed the partitions would be on by default. That's not
always the case apparently (ChromeOS does some funky things here). Both
the SATA and XUSB drivers that have been posted use them as well and the
nouveau driver that Alex has been working on uses at least parts of it.
I don't think it's fair to keep them from being merged while we're
trying to make the transition to power domains, but we should keep an
eye on what's happening there so it doesn't conflict with any of the
work we're planning for power domains.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140709/b4f627bb/attachment.sig>

[PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Peter De Schrijver]

On Wed, Jul 09, 2014 at 04:42:18PM +0200, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Wed, Jul 09, 2014 at 04:20:10PM +0300, Peter De Schrijver wrote:
> > On Wed, Jul 09, 2014 at 02:56:14PM +0200, Thierry Reding wrote:
> > > > Old Signed by an unknown key
> > > 
> > > On Wed, Jul 09, 2014 at 03:43:44PM +0300, Peter De Schrijver wrote:
> > > > On Wed, Jul 09, 2014 at 02:04:02PM +0200, Thierry Reding wrote:
> > > > > > For those 2 domains we can find the necessary clocks and resets by parsing
> > > > > > the relevant existing DT nodes for PCIe and gr3d. For clocks, this isn't
> > > > > > even needed as we can always register some extra clkdev's to get them. There
> > > > > > is no equivalent for resets so we have to parse the gr3d and pcie DT nodes,
> > > > > > but that's not too bad I think.
> > > > > 
> > > > > Even if we could really do this, at this point I don't see an advantage.
> > > > > All that it would be doing is move to some subsystem that doesn't quite
> > > > > match what we need just for the sake of moving to that subsystem. Having
> > > > > a Tegra-specific API doesn't sound so bad anymore.
> > > > > 
> > > > 
> > > > The advantage would be that we can use LP0/SC7 as a cpuidle state.
> > > 
> > > How is that going to work? And why does it need powergates to be
> > 
> > pm_runtime_get() and pm_runtime_put() hook into genpd. So genpd knows
> > when all devices in a domain are idle. It can then decide to turn off
> > the domain (based on the decision of a per domain governor). If all
> > domains are off (except for the non-powergateable domain), a special cpuidle
> > state can be enabled by genpd which will initiate a transition to LP0 without
> > actually doing a full system suspend.
> 
> Okay, I see.
> 
> > > implemented as power domains? If we switch off power gates, then we need
> > > to restore context in the drivers anyway, therefore I assume .suspend()
> > > and .resume() would need to be called, in which case powergate handling
> > > can surely be done at that stage, can't it?
> > > 
> > 
> > .suspend() and .resume() are not used for this. genpd uses other per device
> > callbacks to save and restore the state which are invoked when the domain
> > is turned off and on (.save_state and .restore_state). The major difference
> > with .suspend() and .resume() is that .suspend() has to perform 3 tasks:
> > prevent any new requests to the driver, finalize or cancel all outstanding
> > requests and save the hw context. .save_state will only be called when the
> > device is idle (based on the refcount controlled by pm_runtime_get() and
> > pm_runtime_put()) which means it only has to handle saving the hw context.
> 
> With the above, would it be possible to make turning off the domain
> conditional on whether or not all devices in the domain implement
> .save_state() and .restore_state()? That would allow us to convert to
> power domains and then stage in context save/restore in drivers (or even
> leave it out if there's not enough to be gained from turning the
> partition off).
> 

Maybe. I would have to check that.

> > > > Also system
> > > > resume from LP0 can be faster as we potentially don't have to resume all
> > > > domains at once.
> > > 
> > > I don't understand what that's got to do with anything. If we call into
> > > the PMC driver explicitly via tegra_powergate_*() functions from driver
> > > code, then we have full control over suspend/resume in the drivers, and
> > > therefore don't need to resume all at once either.
> > 
> > But then we would be duplicating all the bookkeeping required for this? What's
> > the point of that?
> 
> We're doing fine without any bookkeeping currently. I understand that
> this may change eventually, but I'm hesitant to start any conversion
> like this before we don't have a better understanding of how it should
> work (and actual use-cases which we can test). Also we've seen in the
> past that coding things up before we have enough use-cases we're bound
> to fail at coming up with a proper binding and then we have to keep
> carrying loads of code for compatibility.
> 
> So if you're willing to give this a shot, I'm not at all opposed to it
> generally. But we need to make sure that both the binding is reasonably
> future-proof and that we can actually test things like reference-counted
> power domains.
> 
> Now in the meantime there are a bunch of other drivers that will need to
> use the powergate API. DC is one of them. We haven't needed this before
> because we assumed the partitions would be on by default. That's not
> always the case apparently (ChromeOS does some funky things here). Both
> the SATA and XUSB drivers that have been posted use them as well and the
> nouveau driver that Alex has been working on uses at least parts of it.
> I don't think it's fair to keep them from being merged while we're
> trying to make the transition to power domains, but we should keep an
> eye on what's happening there so it doesn't conflict with any of the
> work we're planning for power domains.

The problem with this is that moving to the genpd APIs will become much more
difficult I'm afraid. I think we should maybe just make the pmc driver turn
on all the domains which were turned off by the bootloader. That way the
drivers don't need to handle the powerdomains at all for the time being.

Cheers,

Peter.

[PATCH 7/9] ahci: Increase AHCI_MAX_CLKS to 4

[Mikko Perttunen]

The Tegra AHCI device requires four clocks, so increase the maximum
amount of handled clocks from three to four.

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

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

[PATCH 7/9] ahci: Increase AHCI_MAX_CLKS to 4

[Stephen Warren]

On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> The Tegra AHCI device requires four clocks, so increase the maximum
> amount of handled clocks from three to four.

Tejun,

The SATA driver in patch 8/9 in this series would usually be applied in
your ATA tree. However, it has a lot of compile-time dependencies on
things outside the ATA tree that I expect you don't want to deal with.

If you're OK with patches 7 and 8, could you ack them so that I can
apply them to the Tegra tree?

I assume the patches won't cause any significant conflicts, but if you
need, I can certainly give you a signed tag back to allow you to merge
the driver patch (plus the compile-time dependencies) into your tree
later if needed.

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

[Mikko Perttunen]

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

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 drivers/ata/Kconfig      |   9 ++
 drivers/ata/Makefile     |   1 +
 drivers/ata/ahci_tegra.c | 386 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 396 insertions(+)
 create mode 100644 drivers/ata/ahci_tegra.c

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

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

[Rob Herring]

On Wed, Jun 4, 2014 at 6:32 AM, Mikko Perttunen <mperttunen@nvidia.com> wrote:
> This adds support for the integrated AHCI-compliant Serial ATA
> controller present on the NVIDIA Tegra124 system-on-chip.
>
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---

[...]

> +static int tegra_ahci_probe(struct platform_device *pdev)
> +{
> +       const struct of_device_id *match;
> +       struct ahci_host_priv *hpriv;
> +       struct tegra_ahci_priv *tegra;
> +       int ret;
> +
> +       match = of_match_device(tegra_ahci_of_match, &pdev->dev);

This is not needed.

> +       if (!match)
> +               return -EINVAL;
> +

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

[Mikko Perttunen]

Thanks, will remove.

- Mikko

On 05/06/14 15:18, Rob Herring wrote:
> On Wed, Jun 4, 2014 at 6:32 AM, Mikko Perttunen <mperttunen@nvidia.com> wrote:
>> This adds support for the integrated AHCI-compliant Serial ATA
>> controller present on the NVIDIA Tegra124 system-on-chip.
>>
>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>> ---
>
> [...]
>
>> +static int tegra_ahci_probe(struct platform_device *pdev)
>> +{
>> +       const struct of_device_id *match;
>> +       struct ahci_host_priv *hpriv;
>> +       struct tegra_ahci_priv *tegra;
>> +       int ret;
>> +
>> +       match = of_match_device(tegra_ahci_of_match, &pdev->dev);
>
> This is not needed.
>
>> +       if (!match)
>> +               return -EINVAL;
>> +

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

[Stephen Warren]

On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> This adds support for the integrated AHCI-compliant Serial ATA
> controller present on the NVIDIA Tegra124 system-on-chip.

> diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c

> +static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv)

> +	/* Pad calibration */
> +
> +	ret = tegra_fuse_readl(0x224, &val);
> +	if (ret) {
> +		dev_err(&tegra->pdev->dev,
> +			"failed to read sata calibration fuse: %d\n", ret);
> +		return ret;
> +	}
> +
> +	calib = tegra124_pad_calibration[val];

Shouldn't val be range-checked before blindly using it?

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

[Bartlomiej Zolnierkiewicz]

Hi,

On Wednesday, June 04, 2014 02:32:38 PM Mikko Perttunen wrote:
> This adds support for the integrated AHCI-compliant Serial ATA
> controller present on the NVIDIA Tegra124 system-on-chip.
> 
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
>  drivers/ata/Kconfig      |   9 ++
>  drivers/ata/Makefile     |   1 +
>  drivers/ata/ahci_tegra.c | 386 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 396 insertions(+)
>  create mode 100644 drivers/ata/ahci_tegra.c
> 
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index 7671dba..e65d400 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -141,6 +141,15 @@ config AHCI_SUNXI
>  
>  	  If unsure, say N.
>  
> +config AHCI_TEGRA
> +	tristate "NVIDIA Tegra124 AHCI SATA support"
> +	depends on ARCH_TEGRA
> +	help
> +	  This option enables support for the NVIDIA Tegra124 SoC's
> +	  onboard AHCI SATA.
> +
> +	  If unsure, say N.
> +
>  config AHCI_XGENE
>  	tristate "APM X-Gene 6.0Gbps AHCI SATA host controller support"
>  	depends on PHY_XGENE
> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> index 5a02aee..ae41107 100644
> --- a/drivers/ata/Makefile
> +++ b/drivers/ata/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_AHCI_IMX)		+= ahci_imx.o libahci.o libahci_platform.o
>  obj-$(CONFIG_AHCI_MVEBU)	+= ahci_mvebu.o libahci.o libahci_platform.o
>  obj-$(CONFIG_AHCI_SUNXI)	+= ahci_sunxi.o libahci.o libahci_platform.o
>  obj-$(CONFIG_AHCI_ST)		+= ahci_st.o libahci.o libahci_platform.o
> +obj-$(CONFIG_AHCI_TEGRA)	+= ahci_tegra.o libahci.o libahci_platform.o
>  obj-$(CONFIG_AHCI_XGENE)	+= ahci_xgene.o libahci.o libahci_platform.o
>  
>  # SFF w/ custom DMA
> diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c
> new file mode 100644
> index 0000000..a10aac0
> --- /dev/null
> +++ b/drivers/ata/ahci_tegra.c
> @@ -0,0 +1,386 @@
> +/*
> + * drivers/ata/ahci_tegra.c
> + *
> + * Copyright (c) 2014, NVIDIA CORPORATION.  All rights reserved.
> + *
> + * Author:
> + *	Mikko Perttunen <mperttunen@nvidia.com>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/ahci_platform.h>
> +#include <linux/reset.h>
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/tegra-powergate.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/tegra-soc.h>
> +#include "ahci.h"
> +
> +#define SATA_CONFIGURATION_0				0x180
> +#define		SATA_CONFIGURATION_EN_FPCI		(1<<0)

minor nitpick: BIT() macro can be used here

> +#define SCFG_OFFSET					0x1000
> +
> +#define T_SATA0_CFG_1					0x04
> +#define		T_SATA0_CFG_1_IO_SPACE			(1<<0)
> +#define		T_SATA0_CFG_1_MEMORY_SPACE		(1<<1)
> +#define		T_SATA0_CFG_1_BUS_MASTER		(1<<2)
> +#define		T_SATA0_CFG_1_SERR			(1<<8)

ditto

> +#define T_SATA0_CFG_9					0x24
> +
> +#define SATA_FPCI_BAR5					0x94
> +
> +#define SATA_INTR_MASK					0x188
> +#define		SATA_INTR_MASK_IP_INT_MASK		(1<<16)

ditto

> +#define T_SATA0_AHCI_HBA_CAP_BKDR			0x300
> +
> +#define T_SATA0_BKDOOR_CC				0x4a4
> +
> +#define T_SATA0_CFG_SATA				0x54c
> +#define		T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN	(1<<12)

ditto

> +#define T_SATA0_CFG_MISC				0x550
> +
> +#define T_SATA0_INDEX					0x680
> +
> +#define T_SATA0_CHX_PHY_CTRL1_GEN1			0x690
> +#define		T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK	0xff
> +#define		T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT	0
> +#define		T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK	(0xff<<8)
> +#define		T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT 8
> +
> +#define T_SATA0_CHX_PHY_CTRL1_GEN2			0x694
> +#define		T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_MASK	0xff
> +#define		T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_SHIFT	0
> +#define		T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_MASK	(0xff<<12)
> +#define		T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_SHIFT 12
> +
> +#define T_SATA0_CHX_PHY_CTRL2				0x69c
> +#define		T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1	0x23
> +
> +#define T_SATA0_CHX_PHY_CTRL11				0x6d0
> +#define		T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ	(0x2800<<16)
> +
> +struct sata_pad_calibration {
> +	u8 gen1_tx_amp, gen1_tx_peak, gen2_tx_amp, gen2_tx_peak;
> +};
> +
> +static const struct sata_pad_calibration tegra124_pad_calibration[] = {
> +	{0x18, 0x04, 0x18, 0x0a},
> +	{0x0e, 0x04, 0x14, 0x0a},
> +	{0x0e, 0x07, 0x1a, 0x0e},
> +	{0x14, 0x0e, 0x1a, 0x0e},
> +};
> +
> +struct tegra_ahci_priv {
> +	struct platform_device *pdev;
> +	void __iomem *sata_regs;
> +	struct reset_control *sata_rst;
> +	struct reset_control *sata_oob_rst;
> +	struct reset_control *sata_cold_rst;
> +	struct regulator_bulk_data supplies[3];
> +};
> +
> +static inline void sata_writel(struct tegra_ahci_priv *tegra, u32 value,
> +				 unsigned long offset)
> +{
> +	writel(value, tegra->sata_regs + offset);
> +}
> +
> +static inline u32 sata_readl(struct tegra_ahci_priv *tegra,
> +				unsigned long offset)
> +{
> +	return readl(tegra->sata_regs + offset);
> +}

sata_writel() and sata_read() static inlines don't seem to add any value.

Can they be removed?

> +static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
> +{
> +	struct tegra_ahci_priv *tegra = hpriv->plat_data;
> +	int ret;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(tegra->supplies),
> +				    tegra->supplies);
> +	if (ret)
> +		return ret;
> +
> +	reset_control_assert(tegra->sata_rst);
> +	reset_control_assert(tegra->sata_oob_rst);
> +	reset_control_assert(tegra->sata_cold_rst);
> +
> +	ret = tegra_powergate_power_on(TEGRA_POWERGATE_SATA);
> +	if (ret)
> +		goto reset_deassert;

Shouldn't the driver disable regulators on failure?

[ tegra_ahci_power_off() does it so it should also be done here. ]

> +	/* Enable clocks */
> +	ret = ahci_platform_enable_resources(hpriv);
> +	if (ret)
> +		goto power_off;
> +
> +	udelay(10);
> +
> +	ret = tegra_powergate_remove_clamping(TEGRA_POWERGATE_SATA);
> +	if (ret)
> +		goto disable_resources;
> +
> +	udelay(10);
> +
> +	reset_control_deassert(tegra->sata_cold_rst);
> +	reset_control_deassert(tegra->sata_oob_rst);
> +	reset_control_deassert(tegra->sata_rst);
> +
> +	return 0;
> +
> +disable_resources:
> +	ahci_platform_disable_resources(hpriv);
> +
> +power_off:
> +	tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
> +
> +reset_deassert:
> +	reset_control_deassert(tegra->sata_cold_rst);
> +	reset_control_deassert(tegra->sata_oob_rst);
> +	reset_control_deassert(tegra->sata_rst);

reset_control_deassert() sequence is can be moved to separate static
inline helper to prevent code duplication.

> +	return ret;
> +}
> +
> +static void tegra_ahci_power_off(struct ahci_host_priv *hpriv)
> +{
> +	struct tegra_ahci_priv *tegra = hpriv->plat_data;
> +
> +	reset_control_assert(tegra->sata_rst);
> +	reset_control_assert(tegra->sata_oob_rst);
> +	reset_control_assert(tegra->sata_cold_rst);

Same for reset_control_assert() sequence.

> +	ahci_platform_disable_resources(hpriv);
> +
> +	tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
> +
> +	reset_control_deassert(tegra->sata_cold_rst);
> +	reset_control_deassert(tegra->sata_oob_rst);
> +	reset_control_deassert(tegra->sata_rst);
> +
> +	regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra->supplies);
> +}
> +
> +static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv)
> +{
> +	struct tegra_ahci_priv *tegra = hpriv->plat_data;
> +	int ret;
> +	unsigned int val;
> +	struct sata_pad_calibration calib;
> +
> +	ret = tegra_ahci_power_on(hpriv);
> +	if (ret) {
> +		dev_err(&tegra->pdev->dev,
> +			"failed to power on ahci controller: %d\n", ret);
> +		return ret;
> +	}
> +
> +	val = sata_readl(tegra, SATA_CONFIGURATION_0);
> +	val |= SATA_CONFIGURATION_EN_FPCI;
> +	sata_writel(tegra, val, SATA_CONFIGURATION_0);
> +
> +	/* Pad calibration */
> +
> +	ret = tegra_fuse_readl(0x224, &val);
> +	if (ret) {
> +		dev_err(&tegra->pdev->dev,
> +			"failed to read sata calibration fuse: %d\n", ret);
> +		return ret;
> +	}
> +
> +	calib = tegra124_pad_calibration[val];
> +
> +	sata_writel(tegra, (1 << 0), SCFG_OFFSET + T_SATA0_INDEX);
> +
> +	val = sata_readl(tegra, 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;
> +	sata_writel(tegra, val, SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN1);
> +
> +	val = sata_readl(tegra, 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;
> +	sata_writel(tegra, val, SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN2);
> +
> +	sata_writel(tegra, T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ,
> +		    SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL11);
> +	sata_writel(tegra, T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1,
> +		    SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL2);
> +
> +	sata_writel(tegra, 0, SCFG_OFFSET + T_SATA0_INDEX);
> +
> +	/* Program controller device ID */
> +
> +	val = sata_readl(tegra, SCFG_OFFSET + T_SATA0_CFG_SATA);
> +	val |= T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN;
> +	sata_writel(tegra, val, SCFG_OFFSET + T_SATA0_CFG_SATA);
> +
> +	sata_writel(tegra, 0x01060100, SCFG_OFFSET + T_SATA0_BKDOOR_CC);
> +
> +	val = sata_readl(tegra, SCFG_OFFSET + T_SATA0_CFG_SATA);
> +	val &= ~T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN;
> +	sata_writel(tegra, val, SCFG_OFFSET + T_SATA0_CFG_SATA);
> +
> +	/* Enable IO & memory access, bus master mode */
> +
> +	val = sata_readl(tegra, 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;
> +	sata_writel(tegra, val, SCFG_OFFSET + T_SATA0_CFG_1);
> +
> +	/* Program SATA MMIO */
> +
> +	sata_writel(tegra, 0x10000 << 4, SATA_FPCI_BAR5);
> +	sata_writel(tegra, 0x08000 << 13, SCFG_OFFSET + T_SATA0_CFG_9);
> +
> +	/* Unmask SATA interrupts */
> +
> +	val = sata_readl(tegra, SATA_INTR_MASK);
> +	val |= SATA_INTR_MASK_IP_INT_MASK;
> +	sata_writel(tegra, val, SATA_INTR_MASK);
> +
> +	return 0;
> +}
> +
> +static void tegra_ahci_controller_deinit(struct ahci_host_priv *hpriv)
> +{
> +	tegra_ahci_power_off(hpriv);
> +}
> +
> +static void tegra_ahci_host_stop(struct ata_host *host)
> +{
> +	struct ahci_host_priv *hpriv = host->private_data;
> +
> +	tegra_ahci_controller_deinit(hpriv);
> +}
> +
> +static struct ata_port_operations ahci_tegra_port_ops = {
> +	.inherits	= &ahci_platform_ops,
> +	.host_stop	= tegra_ahci_host_stop,
> +};
> +
> +static const struct ata_port_info ahci_tegra_port_info = {
> +	.flags		= AHCI_FLAG_COMMON,
> +	.pio_mask	= ATA_PIO4,
> +	.udma_mask	= ATA_UDMA6,
> +	.port_ops	= &ahci_tegra_port_ops,
> +};
> +
> +static const struct of_device_id tegra_ahci_of_match[] = {
> +	{ .compatible = "nvidia,tegra124-ahci" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, tegra_ahci_of_match);
> +
> +static int tegra_ahci_probe(struct platform_device *pdev)
> +{
> +	const struct of_device_id *match;
> +	struct ahci_host_priv *hpriv;
> +	struct tegra_ahci_priv *tegra;
> +	int ret;
> +
> +	match = of_match_device(tegra_ahci_of_match, &pdev->dev);
> +	if (!match)
> +		return -EINVAL;
> +
> +	hpriv = ahci_platform_get_resources(pdev);
> +	if (IS_ERR(hpriv))
> +		return PTR_ERR(hpriv);
> +
> +	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
> +	if (!tegra)
> +		return -ENOMEM;
> +
> +	hpriv->plat_data = tegra;
> +
> +	tegra->pdev = pdev;
> +
> +	tegra->sata_regs = devm_ioremap_resource(&pdev->dev,
> +		platform_get_resource(pdev, IORESOURCE_MEM, 1));
> +	if (IS_ERR(tegra->sata_regs)) {
> +		dev_err(&pdev->dev, "Failed to get SATA IO memory");
> +		return PTR_ERR(tegra->sata_regs);
> +	}
> +
> +	tegra->sata_rst = devm_reset_control_get(&pdev->dev, "sata");
> +	if (IS_ERR(tegra->sata_rst)) {
> +		dev_err(&pdev->dev, "Failed to get sata reset");
> +		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");
> +		return PTR_ERR(tegra->sata_oob_rst);
> +	}
> +
> +	tegra->sata_cold_rst = devm_reset_control_get(&pdev->dev, "sata-cold");
> +	if (IS_ERR(tegra->sata_cold_rst)) {
> +		dev_err(&pdev->dev, "Failed to get sata-cold reset");
> +		return PTR_ERR(tegra->sata_cold_rst);
> +	}
> +
> +	tegra->supplies[0].supply = "avdd";
> +	tegra->supplies[1].supply = "hvdd";
> +	tegra->supplies[2].supply = "vddio";
> +
> +	ret = devm_regulator_bulk_get(&pdev->dev, ARRAY_SIZE(tegra->supplies),
> +					tegra->supplies);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to get regulators");
> +		return ret;
> +	}
> +
> +	ret = tegra_ahci_controller_init(hpriv);
> +	if (ret)
> +		return ret;
> +
> +	ret = ahci_platform_init_host(pdev, hpriv, &ahci_tegra_port_info,
> +				      0, 0, 0);
> +	if (ret) {
> +		tegra_ahci_controller_deinit(hpriv);
> +		return ret;
> +	}
> +
> +	return 0;
> +};
> +
> +static struct platform_driver tegra_ahci_driver = {
> +	.probe = tegra_ahci_probe,
> +	.remove = ata_platform_remove_one,
> +	.driver = {
> +		.name = "tegra-ahci",
> +		.owner = THIS_MODULE,
> +		.of_match_table = tegra_ahci_of_match,
> +	},

This driver lacks PM suspend/resume support.  I assume that
the Tegra124 platform also doesn't support suspend/resume yet
(if so a comment in the driver code about this would be useful),
otherwise the driver should be fixed.

> +};
> +module_platform_driver(tegra_ahci_driver);
> +
> +MODULE_AUTHOR("Mikko Perttunen <mperttunen@nvidia.com>");
> +MODULE_DESCRIPTION("Tegra124 AHCI SATA driver");
> +MODULE_LICENSE("GPL v2");

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

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

[Stephen Warren]

On 06/17/2014 06:14 AM, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Wednesday, June 04, 2014 02:32:38 PM Mikko Perttunen wrote:
>> This adds support for the integrated AHCI-compliant Serial ATA
>> controller present on the NVIDIA Tegra124 system-on-chip.

>> +static inline void sata_writel(struct tegra_ahci_priv *tegra, u32 value,
>> +				 unsigned long offset)
>> +{
>> +	writel(value, tegra->sata_regs + offset);
>> +}
>> +
>> +static inline u32 sata_readl(struct tegra_ahci_priv *tegra,
>> +				unsigned long offset)
>> +{
>> +	return readl(tegra->sata_regs + offset);
>> +}
> 
> sata_writel() and sata_read() static inlines don't seem to add any value.
> 
> Can they be removed?

Such functions are quite idiomatic in drivers, and serve to simplify all
the call-sites by removing the need to write out the addition of the
base address everywhere.

>> +static struct platform_driver tegra_ahci_driver = {
>> +	.probe = tegra_ahci_probe,
>> +	.remove = ata_platform_remove_one,
>> +	.driver = {
>> +		.name = "tegra-ahci",
>> +		.owner = THIS_MODULE,
>> +		.of_match_table = tegra_ahci_of_match,
>> +	},
> 
> This driver lacks PM suspend/resume support.  I assume that
> the Tegra124 platform also doesn't support suspend/resume yet
> (if so a comment in the driver code about this would be useful),
> otherwise the driver should be fixed.

We do have basic system suspend/resume support. However, I don't think
missing suspend/resume in an individual driver should stop it from being
merged. It can certainly be added later.

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

[Tejun Heo]

On Tue, Jun 17, 2014 at 10:10:23AM -0600, Stephen Warren wrote:
> > sata_writel() and sata_read() static inlines don't seem to add any value.
> > 
> > Can they be removed?
> 
> Such functions are quite idiomatic in drivers, and serve to simplify all
> the call-sites by removing the need to write out the addition of the
> base address everywhere.

I think it obfuscates more than helps.  If you absoluately have to
keep it, please at least name it so that it's clear that it's
something specific to this driver.

Thanks.

-- 
tejun

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

[Tejun Heo]

On Tue, Jun 17, 2014 at 12:13:20PM -0400, Tejun Heo wrote:
> On Tue, Jun 17, 2014 at 10:10:23AM -0600, Stephen Warren wrote:
> > > sata_writel() and sata_read() static inlines don't seem to add any value.
> > > 
> > > Can they be removed?
> > 
> > Such functions are quite idiomatic in drivers, and serve to simplify all
> > the call-sites by removing the need to write out the addition of the
> > base address everywhere.
> 
> I think it obfuscates more than helps.  If you absoluately have to
> keep it, please at least name it so that it's clear that it's
> something specific to this driver.

Please also drop inline.  It isn't a difficult optimization to perform
for the compiler.

-- 
tejun

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

[Stephen Warren]

On 06/17/2014 10:14 AM, Tejun Heo wrote:
> On Tue, Jun 17, 2014 at 12:13:20PM -0400, Tejun Heo wrote:
>> On Tue, Jun 17, 2014 at 10:10:23AM -0600, Stephen Warren wrote:
>>>> sata_writel() and sata_read() static inlines don't seem to add any value.
>>>>
>>>> Can they be removed?
>>>
>>> Such functions are quite idiomatic in drivers, and serve to simplify all
>>> the call-sites by removing the need to write out the addition of the
>>> base address everywhere.
>>
>> I think it obfuscates more than helps.  If you absoluately have to
>> keep it, please at least name it so that it's clear that it's
>> something specific to this driver.
> 
> Please also drop inline.  It isn't a difficult optimization to perform
> for the compiler.

There are probably hundreds of drivers marking those functions inline.
If they aren't marked inline, it'll just stick out as unusual when
people read them all, and then they'll send patches to fix it.

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

[Tejun Heo]

On Tue, Jun 17, 2014 at 10:23:01AM -0600, Stephen Warren wrote:
> > Please also drop inline.  It isn't a difficult optimization to perform
> > for the compiler.
> 
> There are probably hundreds of drivers marking those functions inline.

We used to need them.  We no longer do.  We're in a very long
transition phase.

> If they aren't marked inline, it'll just stick out as unusual when
> people read them all, and then they'll send patches to fix it.

Don't worry.  They won't be applied.

Thanks.

-- 
tejun

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

[Bartlomiej Zolnierkiewicz]

Hi,

On Tuesday, June 17, 2014 10:10:23 AM Stephen Warren wrote:
> On 06/17/2014 06:14 AM, Bartlomiej Zolnierkiewicz wrote:

[...]

> >> +static struct platform_driver tegra_ahci_driver = {
> >> +	.probe = tegra_ahci_probe,
> >> +	.remove = ata_platform_remove_one,
> >> +	.driver = {
> >> +		.name = "tegra-ahci",
> >> +		.owner = THIS_MODULE,
> >> +		.of_match_table = tegra_ahci_of_match,
> >> +	},
> > 
> > This driver lacks PM suspend/resume support.  I assume that
> > the Tegra124 platform also doesn't support suspend/resume yet
> > (if so a comment in the driver code about this would be useful),
> > otherwise the driver should be fixed.
> 
> We do have basic system suspend/resume support. However, I don't think
> missing suspend/resume in an individual driver should stop it from being
> merged. It can certainly be added later.

There should be at least FIXME in the driver explaining the situation and
I would really prefer to have PM support added when the driver is still
"hot" (meaning there are people actively working on it) instead of possibly
having to chase people months/years later when they have already moved on
and are working on something else.  Please also note that adding PM support
should be quite simple if the driver is designed correctly.

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

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

[Mikko Perttunen]

On 06/17/2014 08:04 PM, Bartlomiej Zolnierkiewicz wrote:
>
> Hi,
>
> On Tuesday, June 17, 2014 10:10:23 AM Stephen Warren wrote:
>> On 06/17/2014 06:14 AM, Bartlomiej Zolnierkiewicz wrote:
>
> [...]
>
>>>> +static struct platform_driver tegra_ahci_driver = {
>>>> +	.probe = tegra_ahci_probe,
>>>> +	.remove = ata_platform_remove_one,
>>>> +	.driver = {
>>>> +		.name = "tegra-ahci",
>>>> +		.owner = THIS_MODULE,
>>>> +		.of_match_table = tegra_ahci_of_match,
>>>> +	},
>>>
>>> This driver lacks PM suspend/resume support.  I assume that
>>> the Tegra124 platform also doesn't support suspend/resume yet
>>> (if so a comment in the driver code about this would be useful),
>>> otherwise the driver should be fixed.
>>
>> We do have basic system suspend/resume support. However, I don't think
>> missing suspend/resume in an individual driver should stop it from being
>> merged. It can certainly be added later.
>
> There should be at least FIXME in the driver explaining the situation and
> I would really prefer to have PM support added when the driver is still
> "hot" (meaning there are people actively working on it) instead of possibly
> having to chase people months/years later when they have already moved on
> and are working on something else.  Please also note that adding PM support
> should be quite simple if the driver is designed correctly.
>
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics

AFAIK, the deepest level of suspend currently supported on upstream is 
LP1, for which the driver doesn't need to do anything. Only when we go 
to LP0 the driver will need to save/reload registers and stuff.

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

[Stephen Warren]

On 06/17/2014 11:36 AM, Mikko Perttunen wrote:
> On 06/17/2014 08:04 PM, Bartlomiej Zolnierkiewicz wrote:
>>
>> Hi,
>>
>> On Tuesday, June 17, 2014 10:10:23 AM Stephen Warren wrote:
>>> On 06/17/2014 06:14 AM, Bartlomiej Zolnierkiewicz wrote:
>>
>> [...]
>>
>>>>> +static struct platform_driver tegra_ahci_driver = {
>>>>> +    .probe = tegra_ahci_probe,
>>>>> +    .remove = ata_platform_remove_one,
>>>>> +    .driver = {
>>>>> +        .name = "tegra-ahci",
>>>>> +        .owner = THIS_MODULE,
>>>>> +        .of_match_table = tegra_ahci_of_match,
>>>>> +    },
>>>>
>>>> This driver lacks PM suspend/resume support.  I assume that
>>>> the Tegra124 platform also doesn't support suspend/resume yet
>>>> (if so a comment in the driver code about this would be useful),
>>>> otherwise the driver should be fixed.
>>>
>>> We do have basic system suspend/resume support. However, I don't think
>>> missing suspend/resume in an individual driver should stop it from being
>>> merged. It can certainly be added later.
>>
>> There should be at least FIXME in the driver explaining the situation and
>> I would really prefer to have PM support added when the driver is still
>> "hot" (meaning there are people actively working on it) instead of
>> possibly
>> having to chase people months/years later when they have already moved on
>> and are working on something else.  Please also note that adding PM
>> support
>> should be quite simple if the driver is designed correctly.
>
> AFAIK, the deepest level of suspend currently supported on upstream is
> LP1, for which the driver doesn't need to do anything. Only when we go
> to LP0 the driver will need to save/reload registers and stuff.

Yes, it's generally true that no SoC register state is lost during
suspend, only CPU state.

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

[Mikko Perttunen]

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

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

diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig
index fb25e29..6516f46 100644
--- a/arch/arm/configs/tegra_defconfig
+++ b/arch/arm/configs/tegra_defconfig
@@ -109,6 +109,9 @@ CONFIG_BLK_DEV_SD=y
 CONFIG_BLK_DEV_SR=y
 CONFIG_SCSI_MULTI_LUN=y
 # CONFIG_SCSI_LOWLEVEL is not set
+CONFIG_ATA=y
+CONFIG_SATA_AHCI=y
+CONFIG_AHCI_TEGRA=y
 CONFIG_NETDEVICES=y
 CONFIG_DUMMY=y
 CONFIG_R8169=y
-- 
1.8.1.5

[PATCH 0/9] Serial ATA support for NVIDIA Tegra124

[Stephen Warren]

On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> Hi,
> 
> This series adds support for the onboard AHCI-compliant Serial ATA
> controller found on Tegra124 systems-on-chip. The controller is
> enabled on Jetson TK1. The series depends on Peter's efuse series 
> and Thierry's yet to be submitted XUSB pinctrl driver.

This series includes patches to a lot of different subsystems. That will
complicate applying it.

Can you write a summary of the *compile time* dependencies, since that
will influence how the patches get merged.

You mentioned that this series depends on efuse and XUSB padctl. Can you
point out which specific parts of this series depend on which of those
two other series, and whether this is a compile-time or run-time dependency.

I hope that the drivers/ata patches, drivers/clk patches, DT, and
defconfig patches can each be applied to their normal tree and don't
depend on each-other at compile-time at all.

At run-time, obviously all the patches are needed to make the code work,
but since this is a new feature, it's fine if this all only works once
everything is merged together in linux-next or Linus's tree.

Thanks.

[PATCH 0/9] Serial ATA support for NVIDIA Tegra124

[Mikko Perttunen]

The only compile-time dependencies here should be that:
- patch 8 of 9 which contains the actual driver depends on patch 6 of 9 
(though only when building as a module) and the efuse series
- patch 2 of 9 refers to the DT node called "padctl", so it requires the 
xusb series. (in the submitted xusb series, the node isn't actually 
named, though. I will fix this in v2)

- Mikko

On 05/06/14 20:29, Stephen Warren wrote:
> On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
>> Hi,
>>
>> This series adds support for the onboard AHCI-compliant Serial ATA
>> controller found on Tegra124 systems-on-chip. The controller is
>> enabled on Jetson TK1. The series depends on Peter's efuse series
>> and Thierry's yet to be submitted XUSB pinctrl driver.
>
> This series includes patches to a lot of different subsystems. That will
> complicate applying it.
>
> Can you write a summary of the *compile time* dependencies, since that
> will influence how the patches get merged.
>
> You mentioned that this series depends on efuse and XUSB padctl. Can you
> point out which specific parts of this series depend on which of those
> two other series, and whether this is a compile-time or run-time dependency.
>
> I hope that the drivers/ata patches, drivers/clk patches, DT, and
> defconfig patches can each be applied to their normal tree and don't
> depend on each-other at compile-time at all.
>
> At run-time, obviously all the patches are needed to make the code work,
> but since this is a new feature, it's fine if this all only works once
> everything is merged together in linux-next or Linus's tree.
>
> Thanks.
>

[PATCH 0/9] Serial ATA support for NVIDIA Tegra124

[Thierry Reding]

On Fri, Jun 06, 2014 at 09:27:07AM +0300, Mikko Perttunen wrote:
> The only compile-time dependencies here should be that:
> - patch 8 of 9 which contains the actual driver depends on patch 6 of 9
> (though only when building as a module) and the efuse series
> - patch 2 of 9 refers to the DT node called "padctl", so it requires the
> xusb series. (in the submitted xusb series, the node isn't actually named,
> though. I will fix this in v2)

My intention was to add the label in the patch which first adds a
reference to the device. In my tree that was the PCIe driver. Given that
it's not clear yet in what order these will be merged, I'm thinking I
should simply add that fragment in the patch that adds the node since we
know that it will eventually be used.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140606/d7574596/attachment-0001.sig>

[PATCH 0/9] Serial ATA support for NVIDIA Tegra124

[Mikko Perttunen]

Yes, that might be the easiest. If you go that way, you should probably 
also add an #include for the XUSB binding include file.

- Mikko

On 06/06/14 10:11, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Fri, Jun 06, 2014 at 09:27:07AM +0300, Mikko Perttunen wrote:
>> The only compile-time dependencies here should be that:
>> - patch 8 of 9 which contains the actual driver depends on patch 6 of 9
>> (though only when building as a module) and the efuse series
>> - patch 2 of 9 refers to the DT node called "padctl", so it requires the
>> xusb series. (in the submitted xusb series, the node isn't actually named,
>> though. I will fix this in v2)
>
> My intention was to add the label in the patch which first adds a
> reference to the device. In my tree that was the PCIe driver. Given that
> it's not clear yet in what order these will be merged, I'm thinking I
> should simply add that fragment in the patch that adds the node since we
> know that it will eventually be used.
>
> Thierry
>
> * Unknown Key
> * 0x7F3EB3A1
>

[PATCH 0/9] Serial ATA support for NVIDIA Tegra124

[Stephen Warren]

On 06/06/2014 12:27 AM, Mikko Perttunen wrote:
> The only compile-time dependencies here should be that:
> - patch 8 of 9 which contains the actual driver depends on patch 6 of 9
> (though only when building as a module) and the efuse series

> - patch 2 of 9 refers to the DT node called "padctl", so it requires the
> xusb series. (in the submitted xusb series, the node isn't actually
> named, though. I will fix this in v2)

Isn't that dependency a run-time dependency?

[PATCH 0/9] Serial ATA support for NVIDIA Tegra124

[Mikko Perttunen]

On 06/09/2014 09:33 PM, Stephen Warren wrote:
> On 06/06/2014 12:27 AM, Mikko Perttunen wrote:
>> The only compile-time dependencies here should be that:
>> - patch 8 of 9 which contains the actual driver depends on patch 6 of 9
>> (though only when building as a module) and the efuse series
>
>> - patch 2 of 9 refers to the DT node called "padctl", so it requires the
>> xusb series. (in the submitted xusb series, the node isn't actually
>> named, though. I will fix this in v2)
>
> Isn't that dependency a run-time dependency?

Well, the kernel will compile but the DTC step will fail if building for 
Tegra.

>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[PATCH 0/9] Serial ATA support for NVIDIA Tegra124

[Thierry Reding]

On Mon, Jun 09, 2014 at 09:49:24PM +0300, Mikko Perttunen wrote:
> On 06/09/2014 09:33 PM, Stephen Warren wrote:
> >On 06/06/2014 12:27 AM, Mikko Perttunen wrote:
> >>The only compile-time dependencies here should be that:
> >>- patch 8 of 9 which contains the actual driver depends on patch 6 of 9
> >>(though only when building as a module) and the efuse series
> >
> >>- patch 2 of 9 refers to the DT node called "padctl", so it requires the
> >>xusb series. (in the submitted xusb series, the node isn't actually
> >>named, though. I will fix this in v2)
> >
> >Isn't that dependency a run-time dependency?
> 
> Well, the kernel will compile but the DTC step will fail if building for
> Tegra.

Since this is all changes to the DTS files, we can resolve that
dependency in the Tegra tree by applying the XUSB padctl series
immediately before this patch, right?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140708/2ed06f04/attachment.sig>

[PATCH 0/9] Serial ATA support for NVIDIA Tegra124

[Mikko Perttunen]

On 08/07/14 16:08, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Mon, Jun 09, 2014 at 09:49:24PM +0300, Mikko Perttunen wrote:
>> On 06/09/2014 09:33 PM, Stephen Warren wrote:
>>> On 06/06/2014 12:27 AM, Mikko Perttunen wrote:
>>>> The only compile-time dependencies here should be that:
>>>> - patch 8 of 9 which contains the actual driver depends on patch 6 of 9
>>>> (though only when building as a module) and the efuse series
>>>
>>>> - patch 2 of 9 refers to the DT node called "padctl", so it requires the
>>>> xusb series. (in the submitted xusb series, the node isn't actually
>>>> named, though. I will fix this in v2)
>>>
>>> Isn't that dependency a run-time dependency?
>>
>> Well, the kernel will compile but the DTC step will fail if building for
>> Tegra.
>
> Since this is all changes to the DTS files, we can resolve that
> dependency in the Tegra tree by applying the XUSB padctl series
> immediately before this patch, right?

Yes.

(Also I just realized that though the fuse series is in -next, the pull 
request was NAK'd for now, so don't merge this before that is sorted out.)

>
> Thierry
>
> * Unknown Key
> * 0x7F3EB3A1
>

- Mikko