[PATCH 0/4] Tegra fixes for v4.8-rc1

[PATCH 0/4] Tegra fixes for v4.8-rc1

[Jon Hunter]

Here are a few fixes for Tegra. The clock fix appears to be a bug that
has been around for long time, but it is a simple one-liner and so I
have included here.

The interrupt polarity is also a longer term bug, but has not been
exposed until changes to the IRQ subsystem in v4.8.

Jon Hunter (4):
  drm/tegra: dc: Don't disable display power partition
  ARM: tegra: Correct polarity for Tegra114 PMIC interrupt
  clk: tegra: Correct bit width for PMC output clock mux
  arm64: tegra: Add clock and reset names for audio powergate

 arch/arm/boot/dts/tegra114-dalmore.dts   |  2 +-
 arch/arm/boot/dts/tegra114-roth.dts      |  2 +-
 arch/arm/boot/dts/tegra114-tn7.dts       |  2 +-
 arch/arm64/boot/dts/nvidia/tegra210.dtsi |  2 ++
 drivers/clk/tegra/clk-tegra-pmc.c        |  2 +-
 drivers/gpu/drm/tegra/dc.c               | 21 ++++++++++++++-------
 6 files changed, 20 insertions(+), 11 deletions(-)

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] drm/tegra: dc: Don't disable display power partition

[Jon Hunter]

Commit 33a8eb8d40ee ("drm/tegra: dc: Implement runtime PM") disables the
display power partition when probing and this causes the Tegra114
Dalmore to hang during boot.

The hang occurs when accessing the MIPI calibration registers (which are
accessed during the configuration of the DSI interface). Ideally the
MIPI driver should manage the power partition itself to ensure it is on
when needed. The problem is that the legacy PMC APIs used for managing
the power partitions do not support reference counting and so this
cannot be easily done currently. Long-term we will migrate devices to
use generic PM domains and such scenarios will be easy to support. For
now fix this by removing the code to turn off the display power
partition when probing the DC and always keep the DC on so that the
power partition is not turned off. This is consistent with how the power
partition was managed prior to this commit.

Please note that for earlier devices such as Tegra114 the MIPI
calibration logic is part of the display power partition, where as for
newer devices, such as Tegra124/210 it is part of the SOR power
partition. Hence, in the long-term is makes more sense to handle such
power partitions via the generic PM domain framework.

Fixes: 33a8eb8d40ee ("drm/tegra: dc: Implement runtime PM")

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---

Please note that the hang is only seen on Tegra114 with v4.8 if the
patch "ARM: tegra: Correct polarity for Tegra114 PMIC interrupt" (2nd
patch in series) is applied without this patch. Without the fix for the
PMIC interrupt polarity the Palmas PMIC probe fails and the display
probe also fails because the regulators are not found.

 drivers/gpu/drm/tegra/dc.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 8495bd01b544..17bd80a745d6 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -1217,8 +1217,6 @@ static void tegra_crtc_disable(struct drm_crtc *crtc)
 
 	tegra_dc_stats_reset(&dc->stats);
 	drm_crtc_vblank_off(crtc);
-
-	pm_runtime_put_sync(dc->dev);
 }
 
 static void tegra_crtc_enable(struct drm_crtc *crtc)
@@ -1228,8 +1226,6 @@ static void tegra_crtc_enable(struct drm_crtc *crtc)
 	struct tegra_dc *dc = to_tegra_dc(crtc);
 	u32 value;
 
-	pm_runtime_get_sync(dc->dev);
-
 	/* initialize display controller */
 	if (dc->syncpt) {
 		u32 syncpt = host1x_syncpt_id(dc->syncpt);
@@ -1997,8 +1993,6 @@ static int tegra_dc_probe(struct platform_device *pdev)
 			dc->powergate = TEGRA_POWERGATE_DIS;
 		else
 			dc->powergate = TEGRA_POWERGATE_DISB;
-
-		tegra_powergate_power_off(dc->powergate);
 	}
 
 	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -2020,6 +2014,11 @@ static int tegra_dc_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, dc);
 	pm_runtime_enable(&pdev->dev);
+	err = pm_runtime_get_sync(&pdev->dev);
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to enable device: %d\n", err);
+		goto rpm_disable;
+	}
 
 	INIT_LIST_HEAD(&dc->client.list);
 	dc->client.ops = &dc_client_ops;
@@ -2029,10 +2028,17 @@ static int tegra_dc_probe(struct platform_device *pdev)
 	if (err < 0) {
 		dev_err(&pdev->dev, "failed to register host1x client: %d\n",
 			err);
-		return err;
+		goto rpm_put;
 	}
 
 	return 0;
+
+rpm_put:
+	pm_runtime_put_sync(&pdev->dev);
+rpm_disable:
+	pm_runtime_disable(&pdev->dev);
+
+	return err;
 }
 
 static int tegra_dc_remove(struct platform_device *pdev)
@@ -2053,6 +2059,7 @@ static int tegra_dc_remove(struct platform_device *pdev)
 		return err;
 	}
 
+	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
 	return 0;
-- 
2.1.4

[PATCH 2/4] ARM: tegra: Correct polarity for Tegra114 PMIC interrupt

[Jon Hunter]

The ARM GIC only supports interrupts with either level-high or
rising-edge types for SPIs. The interrupt type for the Palmas PMIC used
for Tegra114 boards is specified as level-low which is invalid for the
GIC. This has gone undetected because until recently, failures to set
the interrupt type when the interrupts are mapped via firmware (such as
device-tree) have not been reported. Since commits 4b357daed698
("genirq: Look-up trigger type if not specified by caller") and
1e2a7d78499e ("irqdomain: Don't set type when mapping an IRQ"), failure
to set the interrupt type will cause the requesting of the interrupt to
fail and exposing incorrectly configured interrupts.

Please note that although the interrupt type was never being set for the
Palmas PMIC, it was still working fine, because the default type setting
for the interrupt, 'level-high', happen to match the correct type for
the interrupt.

Finally, it should be noted that the Palmas interrupt from the PMIC is
actually 'level-low', however, this interrupt signal is inverted by the
Tegra PMC and so the GIC actually sees a 'level-high' interrupt which is
what should be specified in the device-tree interrupt specifier.

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/tegra114-dalmore.dts | 2 +-
 arch/arm/boot/dts/tegra114-roth.dts    | 2 +-
 arch/arm/boot/dts/tegra114-tn7.dts     | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
index 1dfc492cc004..1444fbd543e7 100644
--- a/arch/arm/boot/dts/tegra114-dalmore.dts
+++ b/arch/arm/boot/dts/tegra114-dalmore.dts
@@ -897,7 +897,7 @@
 		palmas: tps65913@58 {
 			compatible = "ti,palmas";
 			reg = <0x58>;
-			interrupts = <0 86 IRQ_TYPE_LEVEL_LOW>;
+			interrupts = <0 86 IRQ_TYPE_LEVEL_HIGH>;
 
 			#interrupt-cells = <2>;
 			interrupt-controller;
diff --git a/arch/arm/boot/dts/tegra114-roth.dts b/arch/arm/boot/dts/tegra114-roth.dts
index 70cf40996c3f..966a7fc044af 100644
--- a/arch/arm/boot/dts/tegra114-roth.dts
+++ b/arch/arm/boot/dts/tegra114-roth.dts
@@ -802,7 +802,7 @@
 		palmas: pmic@58 {
 			compatible = "ti,palmas";
 			reg = <0x58>;
-			interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_LOW>;
+			interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
 
 			#interrupt-cells = <2>;
 			interrupt-controller;
diff --git a/arch/arm/boot/dts/tegra114-tn7.dts b/arch/arm/boot/dts/tegra114-tn7.dts
index 17dd14545862..a161fa1dfb61 100644
--- a/arch/arm/boot/dts/tegra114-tn7.dts
+++ b/arch/arm/boot/dts/tegra114-tn7.dts
@@ -63,7 +63,7 @@
 		palmas: pmic@58 {
 			compatible = "ti,palmas";
 			reg = <0x58>;
-			interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_LOW>;
+			interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
 
 			#interrupt-cells = <2>;
 			interrupt-controller;
-- 
2.1.4

[PATCH 3/4] clk: tegra: Correct bit width for PMC output clock mux

[Jon Hunter]

The bit field for setting the clock mux for the PMC output clocks is a
2-bit field and has always been a 2-bit field for all Tegra devices that
have these clocks (starting with Tegra30). However, the PMC clock driver
incorrectly specifies that this bit field is 3 bits wide and this causes
other bits in the register to be over-written when setting up the mux.
Therefore, correct the width for PMC clock mux to prevent over-writing
other fields.

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---

I did not bother marking this for stable because it has been around for
such a long time I don't think that this has caused any problems. I only
stumbled across this when dumping the register contents during some
testing. Nonetheless we should correct this.

 drivers/clk/tegra/clk-tegra-pmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/tegra/clk-tegra-pmc.c b/drivers/clk/tegra/clk-tegra-pmc.c
index 91377abfefa1..36469a2ca385 100644
--- a/drivers/clk/tegra/clk-tegra-pmc.c
+++ b/drivers/clk/tegra/clk-tegra-pmc.c
@@ -97,7 +97,7 @@ void __init tegra_pmc_clk_init(void __iomem *pmc_base,
 		clk = clk_register_mux(NULL, data->mux_name, data->parents,
 				data->num_parents, CLK_SET_RATE_NO_REPARENT,
 				pmc_base + PMC_CLK_OUT_CNTRL, data->mux_shift,
-				3, 0, &clk_out_lock);
+				2, 0, &clk_out_lock);
 		*dt_clk = clk;
 
 
-- 
2.1.4

[PATCH 4/4] arm64: tegra: Add clock and reset names for audio powergate

[Jon Hunter]

Add the clock and reset names for the Tegra210 Audio powergate. Please
note that these are not currently used, but added from completeness and
to be consistent with the other powergate nodes.

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---

We added the clock and resets names for the XUSB partitions and so I
thought we should do the same for the Audio. Should I update the binding
documentation to say these should be provided? Currently it does not
state these are required. If we do update the binding doc, I was not
entirely sure what to put in the description for these properties.
Typically the clock and reset names would be fixed for a device, but here
we are not using them and so there are no fixed names.

 arch/arm64/boot/dts/nvidia/tegra210.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra210.dtsi b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
index c4cfdcf60d26..92e987acc551 100644
--- a/arch/arm64/boot/dts/nvidia/tegra210.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
@@ -644,7 +644,9 @@
 			pd_audio: aud {
 				clocks = <&tegra_car TEGRA210_CLK_APE>,
 					 <&tegra_car TEGRA210_CLK_APB2APE>;
+				clock-names = "ape", "apb2ape";
 				resets = <&tegra_car 198>;
+				reset-names = "ape";
 				#power-domain-cells = <0>;
 			};
 
-- 
2.1.4

Re: [PATCH 4/4] arm64: tegra: Add clock and reset names for audio powergate

[Mark Rutland]

On Tue, Aug 02, 2016 at 11:34:29AM +0100, Jon Hunter wrote:
> Add the clock and reset names for the Tegra210 Audio powergate. Please
> note that these are not currently used, but added from completeness and
> to be consistent with the other powergate nodes.
> 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> 
> We added the clock and resets names for the XUSB partitions and so I
> thought we should do the same for the Audio. Should I update the binding
> documentation to say these should be provided? Currently it does not
> state these are required. If we do update the binding doc, I was not
> entirely sure what to put in the description for these properties.
> Typically the clock and reset names would be fixed for a device, but here
> we are not using them and so there are no fixed names.

The names are there for the consumer's benefit, so if the consumer
binding doesn't require specific names, they shouldn't be in the DT.
It doesn't make sense to have names which are not fixed from the
consumer's PoV, so if anything the above is an argument for *removing*
the unnecessary and varied names from other nodes.

Thanks,
Mark.

> 
>  arch/arm64/boot/dts/nvidia/tegra210.dtsi | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/nvidia/tegra210.dtsi b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
> index c4cfdcf60d26..92e987acc551 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra210.dtsi
> +++ b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
> @@ -644,7 +644,9 @@
>  			pd_audio: aud {
>  				clocks = <&tegra_car TEGRA210_CLK_APE>,
>  					 <&tegra_car TEGRA210_CLK_APB2APE>;
> +				clock-names = "ape", "apb2ape";
>  				resets = <&tegra_car 198>;
> +				reset-names = "ape";
>  				#power-domain-cells = <0>;
>  			};
>  
> -- 
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] arm64: tegra: Add clock and reset names for audio powergate

[Jon Hunter]

On 02/08/16 15:18, Mark Rutland wrote:
> On Tue, Aug 02, 2016 at 11:34:29AM +0100, Jon Hunter wrote:
>> Add the clock and reset names for the Tegra210 Audio powergate. Please
>> note that these are not currently used, but added from completeness and
>> to be consistent with the other powergate nodes.
>>
>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>
>> We added the clock and resets names for the XUSB partitions and so I
>> thought we should do the same for the Audio. Should I update the binding
>> documentation to say these should be provided? Currently it does not
>> state these are required. If we do update the binding doc, I was not
>> entirely sure what to put in the description for these properties.
>> Typically the clock and reset names would be fixed for a device, but here
>> we are not using them and so there are no fixed names.
> 
> The names are there for the consumer's benefit, so if the consumer
> binding doesn't require specific names, they shouldn't be in the DT.
> It doesn't make sense to have names which are not fixed from the
> consumer's PoV, so if anything the above is an argument for *removing*
> the unnecessary and varied names from other nodes.

Yes I was not sure if this correct. We could always add the names at a
later stage if they are needed.

Thierry, do you still want to have the names present? If not, then I can
remove those for xusb.

Cheers
Jon

-- 
nvpublic

Re: [PATCH 1/4] drm/tegra: dc: Don't disable display power partition

[Thierry Reding]

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

On Tue, Aug 02, 2016 at 11:34:26AM +0100, Jon Hunter wrote:
> Commit 33a8eb8d40ee ("drm/tegra: dc: Implement runtime PM") disables the
> display power partition when probing and this causes the Tegra114
> Dalmore to hang during boot.
> 
> The hang occurs when accessing the MIPI calibration registers (which are
> accessed during the configuration of the DSI interface). Ideally the
> MIPI driver should manage the power partition itself to ensure it is on
> when needed. The problem is that the legacy PMC APIs used for managing
> the power partitions do not support reference counting and so this
> cannot be easily done currently. Long-term we will migrate devices to
> use generic PM domains and such scenarios will be easy to support. For
> now fix this by removing the code to turn off the display power
> partition when probing the DC and always keep the DC on so that the
> power partition is not turned off. This is consistent with how the power
> partition was managed prior to this commit.
> 
> Please note that for earlier devices such as Tegra114 the MIPI
> calibration logic is part of the display power partition, where as for
> newer devices, such as Tegra124/210 it is part of the SOR power
> partition. Hence, in the long-term is makes more sense to handle such
> power partitions via the generic PM domain framework.
> 
> Fixes: 33a8eb8d40ee ("drm/tegra: dc: Implement runtime PM")
> 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> 
> Please note that the hang is only seen on Tegra114 with v4.8 if the
> patch "ARM: tegra: Correct polarity for Tegra114 PMIC interrupt" (2nd
> patch in series) is applied without this patch. Without the fix for the
> PMIC interrupt polarity the Palmas PMIC probe fails and the display
> probe also fails because the regulators are not found.
> 
>  drivers/gpu/drm/tegra/dc.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)

I don't think this fixes the problem at the root. After looking at the
code I think what you're seeing is caused by the tegra_mipi_power_up()
call that happens as part of the tegra_mipi_request() from the DSI
driver's ->probe().

Generally there shouldn't be a problem because the display controller
will always get enabled before the encoder (DSI) and hence the power
partition should be enabled when the actual calibration happens. The
fundamental problem in this case is that we're actually powering up
the MIPI calibration logic at the wrong time. So I think what we'll
want for a proper fix is to move all register accesses out of the
tegra_mipi_request() function and add tegra_mipi_enable() and
tegra_mipi_disable() functions that power up and power down the MIPI
calibration logic, respectively. That way we can move all the code
which relies on the power partition into the tegra_dsi_encoder_enable()
and tegra_dsi_encoder_disable() functions.

Perhaps an even better place to call these new functions from would be
the DSI driver's ->suspend() and ->resume() functions.

An added benefit of this will be that the MIPI calibration logic could
be powered off when DSI is disabled (provided no other user requires it
to be powered on), whereas currently it will remain powered even if the
DSI output is off, since the power on happens in ->probe().

I'll go write a patch.

Thierry

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

Re: [PATCH 1/4] drm/tegra: dc: Don't disable display power partition

[Jon Hunter]

On 12/08/16 14:46, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Aug 02, 2016 at 11:34:26AM +0100, Jon Hunter wrote:
>> Commit 33a8eb8d40ee ("drm/tegra: dc: Implement runtime PM") disables the
>> display power partition when probing and this causes the Tegra114
>> Dalmore to hang during boot.
>>
>> The hang occurs when accessing the MIPI calibration registers (which are
>> accessed during the configuration of the DSI interface). Ideally the
>> MIPI driver should manage the power partition itself to ensure it is on
>> when needed. The problem is that the legacy PMC APIs used for managing
>> the power partitions do not support reference counting and so this
>> cannot be easily done currently. Long-term we will migrate devices to
>> use generic PM domains and such scenarios will be easy to support. For
>> now fix this by removing the code to turn off the display power
>> partition when probing the DC and always keep the DC on so that the
>> power partition is not turned off. This is consistent with how the power
>> partition was managed prior to this commit.
>>
>> Please note that for earlier devices such as Tegra114 the MIPI
>> calibration logic is part of the display power partition, where as for
>> newer devices, such as Tegra124/210 it is part of the SOR power
>> partition. Hence, in the long-term is makes more sense to handle such
>> power partitions via the generic PM domain framework.
>>
>> Fixes: 33a8eb8d40ee ("drm/tegra: dc: Implement runtime PM")
>>
>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>
>> Please note that the hang is only seen on Tegra114 with v4.8 if the
>> patch "ARM: tegra: Correct polarity for Tegra114 PMIC interrupt" (2nd
>> patch in series) is applied without this patch. Without the fix for the
>> PMIC interrupt polarity the Palmas PMIC probe fails and the display
>> probe also fails because the regulators are not found.
>>
>>  drivers/gpu/drm/tegra/dc.c | 21 ++++++++++++++-------
>>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> I don't think this fixes the problem at the root. After looking at the
> code I think what you're seeing is caused by the tegra_mipi_power_up()
> call that happens as part of the tegra_mipi_request() from the DSI
> driver's ->probe().

No it doesn't. It is more of a bandaid. I think that this issue has
always been there but no exposed until the move to RPM for the DC. I
can't say I was too happy with it!

> Generally there shouldn't be a problem because the display controller
> will always get enabled before the encoder (DSI) and hence the power
> partition should be enabled when the actual calibration happens. The
> fundamental problem in this case is that we're actually powering up
> the MIPI calibration logic at the wrong time. So I think what we'll
> want for a proper fix is to move all register accesses out of the
> tegra_mipi_request() function and add tegra_mipi_enable() and
> tegra_mipi_disable() functions that power up and power down the MIPI
> calibration logic, respectively. That way we can move all the code
> which relies on the power partition into the tegra_dsi_encoder_enable()
> and tegra_dsi_encoder_disable() functions.
> 
> Perhaps an even better place to call these new functions from would be
> the DSI driver's ->suspend() and ->resume() functions.
> 
> An added benefit of this will be that the MIPI calibration logic could
> be powered off when DSI is disabled (provided no other user requires it
> to be powered on), whereas currently it will remain powered even if the
> DSI output is off, since the power on happens in ->probe().
> 
> I'll go write a patch.

Great! Thanks.
Jon

-- 
nvpublic

Re: [PATCH 1/4] drm/tegra: dc: Don't disable display power partition

[Thierry Reding]

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

On Fri, Aug 12, 2016 at 04:02:57PM +0100, Jon Hunter wrote:
> 
> On 12/08/16 14:46, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Tue, Aug 02, 2016 at 11:34:26AM +0100, Jon Hunter wrote:
> >> Commit 33a8eb8d40ee ("drm/tegra: dc: Implement runtime PM") disables the
> >> display power partition when probing and this causes the Tegra114
> >> Dalmore to hang during boot.
> >>
> >> The hang occurs when accessing the MIPI calibration registers (which are
> >> accessed during the configuration of the DSI interface). Ideally the
> >> MIPI driver should manage the power partition itself to ensure it is on
> >> when needed. The problem is that the legacy PMC APIs used for managing
> >> the power partitions do not support reference counting and so this
> >> cannot be easily done currently. Long-term we will migrate devices to
> >> use generic PM domains and such scenarios will be easy to support. For
> >> now fix this by removing the code to turn off the display power
> >> partition when probing the DC and always keep the DC on so that the
> >> power partition is not turned off. This is consistent with how the power
> >> partition was managed prior to this commit.
> >>
> >> Please note that for earlier devices such as Tegra114 the MIPI
> >> calibration logic is part of the display power partition, where as for
> >> newer devices, such as Tegra124/210 it is part of the SOR power
> >> partition. Hence, in the long-term is makes more sense to handle such
> >> power partitions via the generic PM domain framework.
> >>
> >> Fixes: 33a8eb8d40ee ("drm/tegra: dc: Implement runtime PM")
> >>
> >> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >> ---
> >>
> >> Please note that the hang is only seen on Tegra114 with v4.8 if the
> >> patch "ARM: tegra: Correct polarity for Tegra114 PMIC interrupt" (2nd
> >> patch in series) is applied without this patch. Without the fix for the
> >> PMIC interrupt polarity the Palmas PMIC probe fails and the display
> >> probe also fails because the regulators are not found.
> >>
> >>  drivers/gpu/drm/tegra/dc.c | 21 ++++++++++++++-------
> >>  1 file changed, 14 insertions(+), 7 deletions(-)
> > 
> > I don't think this fixes the problem at the root. After looking at the
> > code I think what you're seeing is caused by the tegra_mipi_power_up()
> > call that happens as part of the tegra_mipi_request() from the DSI
> > driver's ->probe().
> 
> No it doesn't. It is more of a bandaid. I think that this issue has
> always been there but no exposed until the move to RPM for the DC. I
> can't say I was too happy with it!
> 
> > Generally there shouldn't be a problem because the display controller
> > will always get enabled before the encoder (DSI) and hence the power
> > partition should be enabled when the actual calibration happens. The
> > fundamental problem in this case is that we're actually powering up
> > the MIPI calibration logic at the wrong time. So I think what we'll
> > want for a proper fix is to move all register accesses out of the
> > tegra_mipi_request() function and add tegra_mipi_enable() and
> > tegra_mipi_disable() functions that power up and power down the MIPI
> > calibration logic, respectively. That way we can move all the code
> > which relies on the power partition into the tegra_dsi_encoder_enable()
> > and tegra_dsi_encoder_disable() functions.
> > 
> > Perhaps an even better place to call these new functions from would be
> > the DSI driver's ->suspend() and ->resume() functions.
> > 
> > An added benefit of this will be that the MIPI calibration logic could
> > be powered off when DSI is disabled (provided no other user requires it
> > to be powered on), whereas currently it will remain powered even if the
> > DSI output is off, since the power on happens in ->probe().
> > 
> > I'll go write a patch.
> 
> Great! Thanks.

I was going to send this out, but EIMT reports that it's still failing.
I'll have to dig out the Dalmore and see what's going on.

Thierry

--- >8 ---
From 325ffd78615e76f04b5225fc1e79921c32037c1f Mon Sep 17 00:00:00 2001
From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Date: Fri, 12 Aug 2016 16:00:53 +0200
Subject: [PATCH] drm/tegra: dsi: Enhance runtime power management

The MIPI DSI output on Tegra SoCs requires some external logic to
calibrate the MIPI pads before a video signal can be transmitted. This
MIPI calibration logic requires to be powered on while the MIPI pads are
being used, which is currently done as part of the DSI driver's probe
implementation.

This is suboptimal because it will leave the MIPI calibration logic
powered up even if the DSI output is never used.

On Tegra114 and earlier this behaviour also causes the driver to hang
while trying to power up the MIPI calibration logic because the power
partition that contains the MIPI calibration logic will be powered on
by the display controller at output pipeline configuration time. Thus
the power up sequence for the MIPI calibration logic happens before
it's power partition is guaranteed to be enabled.

Fix this by splitting up the API into a request/free pair of functions
that manage the runtime dependency between the DSI and the calibration
modules (no registers are accessed) and a set of enable, calibrate and
disable functions that program the MIPI calibration logic at points in
time where the power partition is really enabled.

Reported-by: Jonathan Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/gpu/drm/tegra/dsi.c | 14 ++++++++++
 drivers/gpu/host1x/mipi.c   | 63 ++++++++++++++++++++++-----------------------
 include/linux/host1x.h      |  2 ++
 3 files changed, 47 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index 3d228ad90e0f..39a34d1c4d6e 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -1593,6 +1593,12 @@ static int tegra_dsi_suspend(struct device *dev)
 	struct tegra_dsi *dsi = dev_get_drvdata(dev);
 	int err;
 
+	err = tegra_mipi_disable(dsi->mipi);
+	if (err < 0) {
+		dev_err(dev, "failed to disable MIPI calibration: %d\n", err);
+		return err;
+	}
+
 	if (dsi->rst) {
 		err = reset_control_assert(dsi->rst);
 		if (err < 0) {
@@ -1644,8 +1650,16 @@ static int tegra_dsi_resume(struct device *dev)
 		}
 	}
 
+	err = tegra_mipi_enable(dsi->mipi);
+	if (err < 0) {
+		dev_err(dev, "failed to enable MIPI calibration: %d\n", err);
+		goto assert_reset;
+	}
+
 	return 0;
 
+assert_reset:
+	reset_control_assert(dsi->rst);
 disable_clk_lp:
 	clk_disable_unprepare(dsi->clk_lp);
 disable_clk:
diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
index 52a6fd224127..e00809d996a2 100644
--- a/drivers/gpu/host1x/mipi.c
+++ b/drivers/gpu/host1x/mipi.c
@@ -242,20 +242,6 @@ struct tegra_mipi_device *tegra_mipi_request(struct device *device)
 	dev->pads = args.args[0];
 	dev->device = device;
 
-	mutex_lock(&dev->mipi->lock);
-
-	if (dev->mipi->usage_count++ == 0) {
-		err = tegra_mipi_power_up(dev->mipi);
-		if (err < 0) {
-			dev_err(dev->mipi->dev,
-				"failed to power up MIPI bricks: %d\n",
-				err);
-			return ERR_PTR(err);
-		}
-	}
-
-	mutex_unlock(&dev->mipi->lock);
-
 	return dev;
 
 put:
@@ -270,29 +256,42 @@ EXPORT_SYMBOL(tegra_mipi_request);
 
 void tegra_mipi_free(struct tegra_mipi_device *device)
 {
-	int err;
+	platform_device_put(device->pdev);
+	kfree(device);
+}
+EXPORT_SYMBOL(tegra_mipi_free);
 
-	mutex_lock(&device->mipi->lock);
+int tegra_mipi_enable(struct tegra_mipi_device *dev)
+{
+	int err = 0;
 
-	if (--device->mipi->usage_count == 0) {
-		err = tegra_mipi_power_down(device->mipi);
-		if (err < 0) {
-			/*
-			 * Not much that can be done here, so an error message
-			 * will have to do.
-			 */
-			dev_err(device->mipi->dev,
-				"failed to power down MIPI bricks: %d\n",
-				err);
-		}
-	}
+	mutex_lock(&dev->mipi->lock);
 
-	mutex_unlock(&device->mipi->lock);
+	if (dev->mipi->usage_count++ == 0)
+		err = tegra_mipi_power_up(dev->mipi);
+
+	mutex_unlock(&dev->mipi->lock);
+
+	return err;
 
-	platform_device_put(device->pdev);
-	kfree(device);
 }
-EXPORT_SYMBOL(tegra_mipi_free);
+EXPORT_SYMBOL(tegra_mipi_enable);
+
+int tegra_mipi_disable(struct tegra_mipi_device *dev)
+{
+	int err = 0;
+
+	mutex_lock(&dev->mipi->lock);
+
+	if (--dev->mipi->usage_count == 0)
+		err = tegra_mipi_power_down(dev->mipi);
+
+	mutex_unlock(&dev->mipi->lock);
+
+	return err;
+
+}
+EXPORT_SYMBOL(tegra_mipi_disable);
 
 static int tegra_mipi_wait(struct tegra_mipi *mipi)
 {
diff --git a/include/linux/host1x.h b/include/linux/host1x.h
index d2ba7d334039..1ffbf2a8cb99 100644
--- a/include/linux/host1x.h
+++ b/include/linux/host1x.h
@@ -304,6 +304,8 @@ struct tegra_mipi_device;
 
 struct tegra_mipi_device *tegra_mipi_request(struct device *device);
 void tegra_mipi_free(struct tegra_mipi_device *device);
+int tegra_mipi_enable(struct tegra_mipi_device *device);
+int tegra_mipi_disable(struct tegra_mipi_device *device);
 int tegra_mipi_calibrate(struct tegra_mipi_device *device);
 
 #endif
-- 
2.9.0


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

Re: [PATCH 3/4] clk: tegra: Correct bit width for PMC output clock mux

[Thierry Reding]

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

On Tue, Aug 02, 2016 at 11:34:28AM +0100, Jon Hunter wrote:
> The bit field for setting the clock mux for the PMC output clocks is a
> 2-bit field and has always been a 2-bit field for all Tegra devices that
> have these clocks (starting with Tegra30). However, the PMC clock driver
> incorrectly specifies that this bit field is 3 bits wide and this causes
> other bits in the register to be over-written when setting up the mux.
> Therefore, correct the width for PMC clock mux to prevent over-writing
> other fields.
> 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> 
> I did not bother marking this for stable because it has been around for
> such a long time I don't think that this has caused any problems. I only
> stumbled across this when dumping the register contents during some
> testing. Nonetheless we should correct this.
> 
>  drivers/clk/tegra/clk-tegra-pmc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks.

Thierry

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

Re: [PATCH 3/4] clk: tegra: Correct bit width for PMC output clock mux

[Thierry Reding]

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

On Tue, Aug 02, 2016 at 11:34:28AM +0100, Jon Hunter wrote:
> The bit field for setting the clock mux for the PMC output clocks is a
> 2-bit field and has always been a 2-bit field for all Tegra devices that
> have these clocks (starting with Tegra30). However, the PMC clock driver
> incorrectly specifies that this bit field is 3 bits wide and this causes
> other bits in the register to be over-written when setting up the mux.
> Therefore, correct the width for PMC clock mux to prevent over-writing
> other fields.
> 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> 
> I did not bother marking this for stable because it has been around for
> such a long time I don't think that this has caused any problems. I only
> stumbled across this when dumping the register contents during some
> testing. Nonetheless we should correct this.
> 
>  drivers/clk/tegra/clk-tegra-pmc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks.

Thierry

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

Re: [PATCH 4/4] arm64: tegra: Add clock and reset names for audio powergate

[Thierry Reding]

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

On Tue, Aug 02, 2016 at 07:43:41PM +0100, Jon Hunter wrote:
> 
> On 02/08/16 15:18, Mark Rutland wrote:
> > On Tue, Aug 02, 2016 at 11:34:29AM +0100, Jon Hunter wrote:
> >> Add the clock and reset names for the Tegra210 Audio powergate. Please
> >> note that these are not currently used, but added from completeness and
> >> to be consistent with the other powergate nodes.
> >>
> >> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >> ---
> >>
> >> We added the clock and resets names for the XUSB partitions and so I
> >> thought we should do the same for the Audio. Should I update the binding
> >> documentation to say these should be provided? Currently it does not
> >> state these are required. If we do update the binding doc, I was not
> >> entirely sure what to put in the description for these properties.
> >> Typically the clock and reset names would be fixed for a device, but here
> >> we are not using them and so there are no fixed names.
> > 
> > The names are there for the consumer's benefit, so if the consumer
> > binding doesn't require specific names, they shouldn't be in the DT.
> > It doesn't make sense to have names which are not fixed from the
> > consumer's PoV, so if anything the above is an argument for *removing*
> > the unnecessary and varied names from other nodes.
> 
> Yes I was not sure if this correct. We could always add the names at a
> later stage if they are needed.
> 
> Thierry, do you still want to have the names present? If not, then I can
> remove those for xusb.

I would've liked to keep this consistent with other nodes, but as Mark
pointed out the power domain case is somewhat special in that the names
or any ordering are completely irrelevant.

Let's drop this one and remove the clock and reset names from the XUSB
power domains as well.

Thierry

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