[V6,01/21] irqchip: tegra: Do not disable COP IRQ during suspend
diff mbox series

Message ID 1563738060-30213-2-git-send-email-skomatineni@nvidia.com
State New
Headers show
Series
  • SC7 entry and exit support for Tegra210
Related show

Commit Message

Sowjanya Komatineni July 21, 2019, 7:40 p.m. UTC
Tegra210 platforms use sc7 entry firmware to program Tegra LP0/SC7 entry
sequence and sc7 entry firmware is run from COP/BPMP-Lite.

So, COP/BPMP-Lite still need IRQ function to finish SC7 suspend sequence
for Tegra210.

This patch has fix for leaving the COP IRQ enabled for Tegra210 during
interrupt controller suspend operation.

Acked-by: Thierry Reding <treding@nvidia.com>
Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 drivers/irqchip/irq-tegra.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

Comments

Marc Zyngier July 21, 2019, 8:24 p.m. UTC | #1
On Sun, 21 Jul 2019 12:40:40 -0700
Sowjanya Komatineni <skomatineni@nvidia.com> wrote:

> Tegra210 platforms use sc7 entry firmware to program Tegra LP0/SC7 entry
> sequence and sc7 entry firmware is run from COP/BPMP-Lite.
> 
> So, COP/BPMP-Lite still need IRQ function to finish SC7 suspend sequence
> for Tegra210.
> 
> This patch has fix for leaving the COP IRQ enabled for Tegra210 during
> interrupt controller suspend operation.
> 
> Acked-by: Thierry Reding <treding@nvidia.com>
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>

Acked-by: Marc Zyngier <maz@kernel.org>

Please let me know how you want this to be merged (either via the
irqchip tree as a standalone patch, or as part of the series via
another tree).

Thanks,

	M.
Dmitry Osipenko July 22, 2019, 9:54 a.m. UTC | #2
21.07.2019 22:40, Sowjanya Komatineni пишет:
> Tegra210 platforms use sc7 entry firmware to program Tegra LP0/SC7 entry
> sequence and sc7 entry firmware is run from COP/BPMP-Lite.
> 
> So, COP/BPMP-Lite still need IRQ function to finish SC7 suspend sequence
> for Tegra210.
> 
> This patch has fix for leaving the COP IRQ enabled for Tegra210 during
> interrupt controller suspend operation.
> 
> Acked-by: Thierry Reding <treding@nvidia.com>
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  drivers/irqchip/irq-tegra.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-tegra.c b/drivers/irqchip/irq-tegra.c
> index e1f771c72fc4..851f88cef508 100644
> --- a/drivers/irqchip/irq-tegra.c
> +++ b/drivers/irqchip/irq-tegra.c
> @@ -44,6 +44,7 @@ static unsigned int num_ictlrs;
>  
>  struct tegra_ictlr_soc {
>  	unsigned int num_ictlrs;
> +	bool supports_sc7;
>  };
>  
>  static const struct tegra_ictlr_soc tegra20_ictlr_soc = {
> @@ -56,6 +57,7 @@ static const struct tegra_ictlr_soc tegra30_ictlr_soc = {
>  
>  static const struct tegra_ictlr_soc tegra210_ictlr_soc = {
>  	.num_ictlrs = 6,
> +	.supports_sc7 = true,
>  };
>  
>  static const struct of_device_id ictlr_matches[] = {
> @@ -67,6 +69,7 @@ static const struct of_device_id ictlr_matches[] = {
>  
>  struct tegra_ictlr_info {
>  	void __iomem *base[TEGRA_MAX_NUM_ICTLRS];
> +	const struct tegra_ictlr_soc *soc;
>  #ifdef CONFIG_PM_SLEEP
>  	u32 cop_ier[TEGRA_MAX_NUM_ICTLRS];
>  	u32 cop_iep[TEGRA_MAX_NUM_ICTLRS];
> @@ -147,8 +150,20 @@ static int tegra_ictlr_suspend(void)
>  		lic->cop_ier[i] = readl_relaxed(ictlr + ICTLR_COP_IER);
>  		lic->cop_iep[i] = readl_relaxed(ictlr + ICTLR_COP_IEP_CLASS);
>  
> -		/* Disable COP interrupts */
> -		writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
> +		/*
> +		 * AVP/COP/BPMP-Lite is the Tegra boot processor.
> +		 *
> +		 * Tegra210 system suspend flow uses sc7entry firmware which
> +		 * is executed by COP/BPMP and it includes disabling COP IRQ,
> +		 * clamping CPU rail, turning off VDD_CPU, and preparing the
> +		 * system to go to SC7/LP0.
> +		 *
> +		 * COP/BPMP wakes up when COP IRQ is triggered and runs
> +		 * sc7entry-firmware. So need to keep COP interrupt enabled.
> +		 */
> +		if (!lic->soc->supports_sc7)
> +			/* Disable COP interrupts if SC7 is not supported */

All Tegra SoCs support SC7, hence the 'supports_sc7' and the comment
doesn't sound correct to me. Something like 'firmware_sc7' should suit
better here.

> +			writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);

Secondly, I'm also not sure why COP interrupts need to be disabled for
pre-T210 at all, since COP is unused. This looks to me like it was
cut-n-pasted from downstream kernel without a good reason and could be
simply removed.

>  		/* Disable CPU interrupts */
>  		writel_relaxed(~0ul, ictlr + ICTLR_CPU_IER_CLR);
> @@ -339,6 +354,7 @@ static int __init tegra_ictlr_init(struct device_node *node,
>  		goto out_unmap;
>  	}
>  
> +	lic->soc = soc;
>  	tegra_ictlr_syscore_init();
>  
>  	pr_info("%pOF: %d interrupts forwarded to %pOF\n",
>
Marc Zyngier July 22, 2019, 10:13 a.m. UTC | #3
On 22/07/2019 10:54, Dmitry Osipenko wrote:
> 21.07.2019 22:40, Sowjanya Komatineni пишет:
>> Tegra210 platforms use sc7 entry firmware to program Tegra LP0/SC7 entry
>> sequence and sc7 entry firmware is run from COP/BPMP-Lite.
>>
>> So, COP/BPMP-Lite still need IRQ function to finish SC7 suspend sequence
>> for Tegra210.
>>
>> This patch has fix for leaving the COP IRQ enabled for Tegra210 during
>> interrupt controller suspend operation.
>>
>> Acked-by: Thierry Reding <treding@nvidia.com>
>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>> ---
>>  drivers/irqchip/irq-tegra.c | 20 ++++++++++++++++++--
>>  1 file changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-tegra.c b/drivers/irqchip/irq-tegra.c
>> index e1f771c72fc4..851f88cef508 100644
>> --- a/drivers/irqchip/irq-tegra.c
>> +++ b/drivers/irqchip/irq-tegra.c
>> @@ -44,6 +44,7 @@ static unsigned int num_ictlrs;
>>  
>>  struct tegra_ictlr_soc {
>>  	unsigned int num_ictlrs;
>> +	bool supports_sc7;
>>  };
>>  
>>  static const struct tegra_ictlr_soc tegra20_ictlr_soc = {
>> @@ -56,6 +57,7 @@ static const struct tegra_ictlr_soc tegra30_ictlr_soc = {
>>  
>>  static const struct tegra_ictlr_soc tegra210_ictlr_soc = {
>>  	.num_ictlrs = 6,
>> +	.supports_sc7 = true,
>>  };
>>  
>>  static const struct of_device_id ictlr_matches[] = {
>> @@ -67,6 +69,7 @@ static const struct of_device_id ictlr_matches[] = {
>>  
>>  struct tegra_ictlr_info {
>>  	void __iomem *base[TEGRA_MAX_NUM_ICTLRS];
>> +	const struct tegra_ictlr_soc *soc;
>>  #ifdef CONFIG_PM_SLEEP
>>  	u32 cop_ier[TEGRA_MAX_NUM_ICTLRS];
>>  	u32 cop_iep[TEGRA_MAX_NUM_ICTLRS];
>> @@ -147,8 +150,20 @@ static int tegra_ictlr_suspend(void)
>>  		lic->cop_ier[i] = readl_relaxed(ictlr + ICTLR_COP_IER);
>>  		lic->cop_iep[i] = readl_relaxed(ictlr + ICTLR_COP_IEP_CLASS);
>>  
>> -		/* Disable COP interrupts */
>> -		writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
>> +		/*
>> +		 * AVP/COP/BPMP-Lite is the Tegra boot processor.
>> +		 *
>> +		 * Tegra210 system suspend flow uses sc7entry firmware which
>> +		 * is executed by COP/BPMP and it includes disabling COP IRQ,
>> +		 * clamping CPU rail, turning off VDD_CPU, and preparing the
>> +		 * system to go to SC7/LP0.
>> +		 *
>> +		 * COP/BPMP wakes up when COP IRQ is triggered and runs
>> +		 * sc7entry-firmware. So need to keep COP interrupt enabled.
>> +		 */
>> +		if (!lic->soc->supports_sc7)
>> +			/* Disable COP interrupts if SC7 is not supported */
> 
> All Tegra SoCs support SC7, hence the 'supports_sc7' and the comment
> doesn't sound correct to me. Something like 'firmware_sc7' should suit
> better here.

If what you're saying is true, then the whole patch is wrong, and the
SC7 property should come from DT.

> 
>> +			writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
> 
> Secondly, I'm also not sure why COP interrupts need to be disabled for
> pre-T210 at all, since COP is unused. This looks to me like it was
> cut-n-pasted from downstream kernel without a good reason and could be
> simply removed.

Please verify that this is actually the case. Tegra-2 definitely needed
some level of poking, and I'm not keen on changing anything there until
you (or someone else) has verified it on actual HW (see e307cc8941fc).

Joseph, can you please shed some light here?

	M.
Dmitry Osipenko July 22, 2019, 10:57 a.m. UTC | #4
22.07.2019 13:13, Marc Zyngier пишет:
> On 22/07/2019 10:54, Dmitry Osipenko wrote:
>> 21.07.2019 22:40, Sowjanya Komatineni пишет:
>>> Tegra210 platforms use sc7 entry firmware to program Tegra LP0/SC7 entry
>>> sequence and sc7 entry firmware is run from COP/BPMP-Lite.
>>>
>>> So, COP/BPMP-Lite still need IRQ function to finish SC7 suspend sequence
>>> for Tegra210.
>>>
>>> This patch has fix for leaving the COP IRQ enabled for Tegra210 during
>>> interrupt controller suspend operation.
>>>
>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>> ---
>>>  drivers/irqchip/irq-tegra.c | 20 ++++++++++++++++++--
>>>  1 file changed, 18 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-tegra.c b/drivers/irqchip/irq-tegra.c
>>> index e1f771c72fc4..851f88cef508 100644
>>> --- a/drivers/irqchip/irq-tegra.c
>>> +++ b/drivers/irqchip/irq-tegra.c
>>> @@ -44,6 +44,7 @@ static unsigned int num_ictlrs;
>>>  
>>>  struct tegra_ictlr_soc {
>>>  	unsigned int num_ictlrs;
>>> +	bool supports_sc7;
>>>  };
>>>  
>>>  static const struct tegra_ictlr_soc tegra20_ictlr_soc = {
>>> @@ -56,6 +57,7 @@ static const struct tegra_ictlr_soc tegra30_ictlr_soc = {
>>>  
>>>  static const struct tegra_ictlr_soc tegra210_ictlr_soc = {
>>>  	.num_ictlrs = 6,
>>> +	.supports_sc7 = true,
>>>  };
>>>  
>>>  static const struct of_device_id ictlr_matches[] = {
>>> @@ -67,6 +69,7 @@ static const struct of_device_id ictlr_matches[] = {
>>>  
>>>  struct tegra_ictlr_info {
>>>  	void __iomem *base[TEGRA_MAX_NUM_ICTLRS];
>>> +	const struct tegra_ictlr_soc *soc;
>>>  #ifdef CONFIG_PM_SLEEP
>>>  	u32 cop_ier[TEGRA_MAX_NUM_ICTLRS];
>>>  	u32 cop_iep[TEGRA_MAX_NUM_ICTLRS];
>>> @@ -147,8 +150,20 @@ static int tegra_ictlr_suspend(void)
>>>  		lic->cop_ier[i] = readl_relaxed(ictlr + ICTLR_COP_IER);
>>>  		lic->cop_iep[i] = readl_relaxed(ictlr + ICTLR_COP_IEP_CLASS);
>>>  
>>> -		/* Disable COP interrupts */
>>> -		writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
>>> +		/*
>>> +		 * AVP/COP/BPMP-Lite is the Tegra boot processor.
>>> +		 *
>>> +		 * Tegra210 system suspend flow uses sc7entry firmware which
>>> +		 * is executed by COP/BPMP and it includes disabling COP IRQ,
>>> +		 * clamping CPU rail, turning off VDD_CPU, and preparing the
>>> +		 * system to go to SC7/LP0.
>>> +		 *
>>> +		 * COP/BPMP wakes up when COP IRQ is triggered and runs
>>> +		 * sc7entry-firmware. So need to keep COP interrupt enabled.
>>> +		 */
>>> +		if (!lic->soc->supports_sc7)
>>> +			/* Disable COP interrupts if SC7 is not supported */
>>
>> All Tegra SoCs support SC7, hence the 'supports_sc7' and the comment
>> doesn't sound correct to me. Something like 'firmware_sc7' should suit
>> better here.
> 
> If what you're saying is true, then the whole patch is wrong, and the
> SC7 property should come from DT.

It should be safe to assume that all of existing Tegra210 devices use
the firmware for SC7, hence I wouldn't say that the patch is entirely
wrong. To me it's not entirely correct.

>>
>>> +			writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
>>
>> Secondly, I'm also not sure why COP interrupts need to be disabled for
>> pre-T210 at all, since COP is unused. This looks to me like it was
>> cut-n-pasted from downstream kernel without a good reason and could be
>> simply removed.
> 
> Please verify that this is actually the case. Tegra-2 definitely needed
> some level of poking, and I'm not keen on changing anything there until
> you (or someone else) has verified it on actual HW (see e307cc8941fc).

Tested on Tegra20 and Tegra30, LP1 suspend-resume works perfectly fine
with all COP bits removed from the driver.

AFAIK, the reason why downstream needed that disabling is that it uses
proprietary firmware which is running on the COP and that firmware is
usually a BLOB audio/video DEC-ENC driver which doesn't cleanup
interrupts after itself. That firmware is not applicable for the
upstream kernel, hence there is no need to care about it.

> Joseph, can you please shed some light here?
Sowjanya Komatineni July 22, 2019, 4:21 p.m. UTC | #5
On 7/22/19 3:57 AM, Dmitry Osipenko wrote:
> 22.07.2019 13:13, Marc Zyngier пишет:
>> On 22/07/2019 10:54, Dmitry Osipenko wrote:
>>> 21.07.2019 22:40, Sowjanya Komatineni пишет:
>>>> Tegra210 platforms use sc7 entry firmware to program Tegra LP0/SC7 entry
>>>> sequence and sc7 entry firmware is run from COP/BPMP-Lite.
>>>>
>>>> So, COP/BPMP-Lite still need IRQ function to finish SC7 suspend sequence
>>>> for Tegra210.
>>>>
>>>> This patch has fix for leaving the COP IRQ enabled for Tegra210 during
>>>> interrupt controller suspend operation.
>>>>
>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>> ---
>>>>   drivers/irqchip/irq-tegra.c | 20 ++++++++++++++++++--
>>>>   1 file changed, 18 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/irqchip/irq-tegra.c b/drivers/irqchip/irq-tegra.c
>>>> index e1f771c72fc4..851f88cef508 100644
>>>> --- a/drivers/irqchip/irq-tegra.c
>>>> +++ b/drivers/irqchip/irq-tegra.c
>>>> @@ -44,6 +44,7 @@ static unsigned int num_ictlrs;
>>>>   
>>>>   struct tegra_ictlr_soc {
>>>>   	unsigned int num_ictlrs;
>>>> +	bool supports_sc7;
>>>>   };
>>>>   
>>>>   static const struct tegra_ictlr_soc tegra20_ictlr_soc = {
>>>> @@ -56,6 +57,7 @@ static const struct tegra_ictlr_soc tegra30_ictlr_soc = {
>>>>   
>>>>   static const struct tegra_ictlr_soc tegra210_ictlr_soc = {
>>>>   	.num_ictlrs = 6,
>>>> +	.supports_sc7 = true,
>>>>   };
>>>>   
>>>>   static const struct of_device_id ictlr_matches[] = {
>>>> @@ -67,6 +69,7 @@ static const struct of_device_id ictlr_matches[] = {
>>>>   
>>>>   struct tegra_ictlr_info {
>>>>   	void __iomem *base[TEGRA_MAX_NUM_ICTLRS];
>>>> +	const struct tegra_ictlr_soc *soc;
>>>>   #ifdef CONFIG_PM_SLEEP
>>>>   	u32 cop_ier[TEGRA_MAX_NUM_ICTLRS];
>>>>   	u32 cop_iep[TEGRA_MAX_NUM_ICTLRS];
>>>> @@ -147,8 +150,20 @@ static int tegra_ictlr_suspend(void)
>>>>   		lic->cop_ier[i] = readl_relaxed(ictlr + ICTLR_COP_IER);
>>>>   		lic->cop_iep[i] = readl_relaxed(ictlr + ICTLR_COP_IEP_CLASS);
>>>>   
>>>> -		/* Disable COP interrupts */
>>>> -		writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
>>>> +		/*
>>>> +		 * AVP/COP/BPMP-Lite is the Tegra boot processor.
>>>> +		 *
>>>> +		 * Tegra210 system suspend flow uses sc7entry firmware which
>>>> +		 * is executed by COP/BPMP and it includes disabling COP IRQ,
>>>> +		 * clamping CPU rail, turning off VDD_CPU, and preparing the
>>>> +		 * system to go to SC7/LP0.
>>>> +		 *
>>>> +		 * COP/BPMP wakes up when COP IRQ is triggered and runs
>>>> +		 * sc7entry-firmware. So need to keep COP interrupt enabled.
>>>> +		 */
>>>> +		if (!lic->soc->supports_sc7)
>>>> +			/* Disable COP interrupts if SC7 is not supported */
>>> All Tegra SoCs support SC7, hence the 'supports_sc7' and the comment
>>> doesn't sound correct to me. Something like 'firmware_sc7' should suit
>>> better here.
>> If what you're saying is true, then the whole patch is wrong, and the
>> SC7 property should come from DT.
> It should be safe to assume that all of existing Tegra210 devices use
> the firmware for SC7, hence I wouldn't say that the patch is entirely
> wrong. To me it's not entirely correct.

Yes, all existing Tegra210 platforms uses sc7 entry firmware for SC7 and 
AVP/COP IRQ need to be kept enabled as during suspend ATF triggers IRQ 
to COP for SC7 entry fw execution.


>>>> +			writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
>>> Secondly, I'm also not sure why COP interrupts need to be disabled for
>>> pre-T210 at all, since COP is unused. This looks to me like it was
>>> cut-n-pasted from downstream kernel without a good reason and could be
>>> simply removed.
>> Please verify that this is actually the case. Tegra-2 definitely needed
>> some level of poking, and I'm not keen on changing anything there until
>> you (or someone else) has verified it on actual HW (see e307cc8941fc).
> Tested on Tegra20 and Tegra30, LP1 suspend-resume works perfectly fine
> with all COP bits removed from the driver.
>
> AFAIK, the reason why downstream needed that disabling is that it uses
> proprietary firmware which is running on the COP and that firmware is
> usually a BLOB audio/video DEC-ENC driver which doesn't cleanup
> interrupts after itself. That firmware is not applicable for the
> upstream kernel, hence there is no need to care about it.
>
>> Joseph, can you please shed some light here?

SC7 entry flow uses 3rd party ATF (arm-trusted FW) blob which is the one that actually loads SC7 entry firmware and triggers IRQ to AVP/COP which causes COP to wakeup and run SC7 entry FW.

So when SC7 support is enabled, IRQ need to be kept enabled and when SC7 FW starts execution, it will disable COP IRQ.
Marc Zyngier July 22, 2019, 6:38 p.m. UTC | #6
On Mon, 22 Jul 2019 09:21:21 -0700
Sowjanya Komatineni <skomatineni@nvidia.com> wrote:

> On 7/22/19 3:57 AM, Dmitry Osipenko wrote:
> > 22.07.2019 13:13, Marc Zyngier пишет:  
> >> On 22/07/2019 10:54, Dmitry Osipenko wrote:  
> >>> 21.07.2019 22:40, Sowjanya Komatineni пишет:  
> >>>> Tegra210 platforms use sc7 entry firmware to program Tegra LP0/SC7 entry
> >>>> sequence and sc7 entry firmware is run from COP/BPMP-Lite.
> >>>>
> >>>> So, COP/BPMP-Lite still need IRQ function to finish SC7 suspend sequence
> >>>> for Tegra210.
> >>>>
> >>>> This patch has fix for leaving the COP IRQ enabled for Tegra210 during
> >>>> interrupt controller suspend operation.
> >>>>
> >>>> Acked-by: Thierry Reding <treding@nvidia.com>
> >>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> >>>> ---
> >>>>   drivers/irqchip/irq-tegra.c | 20 ++++++++++++++++++--
> >>>>   1 file changed, 18 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/irqchip/irq-tegra.c b/drivers/irqchip/irq-tegra.c
> >>>> index e1f771c72fc4..851f88cef508 100644
> >>>> --- a/drivers/irqchip/irq-tegra.c
> >>>> +++ b/drivers/irqchip/irq-tegra.c
> >>>> @@ -44,6 +44,7 @@ static unsigned int num_ictlrs;
> >>>>   
> >>>>   struct tegra_ictlr_soc {
> >>>>   	unsigned int num_ictlrs;
> >>>> +	bool supports_sc7;
> >>>>   };
> >>>>   
> >>>>   static const struct tegra_ictlr_soc tegra20_ictlr_soc = {
> >>>> @@ -56,6 +57,7 @@ static const struct tegra_ictlr_soc tegra30_ictlr_soc = {
> >>>>   
> >>>>   static const struct tegra_ictlr_soc tegra210_ictlr_soc = {
> >>>>   	.num_ictlrs = 6,
> >>>> +	.supports_sc7 = true,
> >>>>   };
> >>>>   
> >>>>   static const struct of_device_id ictlr_matches[] = {
> >>>> @@ -67,6 +69,7 @@ static const struct of_device_id ictlr_matches[] = {
> >>>>   
> >>>>   struct tegra_ictlr_info {
> >>>>   	void __iomem *base[TEGRA_MAX_NUM_ICTLRS];
> >>>> +	const struct tegra_ictlr_soc *soc;
> >>>>   #ifdef CONFIG_PM_SLEEP
> >>>>   	u32 cop_ier[TEGRA_MAX_NUM_ICTLRS];
> >>>>   	u32 cop_iep[TEGRA_MAX_NUM_ICTLRS];
> >>>> @@ -147,8 +150,20 @@ static int tegra_ictlr_suspend(void)
> >>>>   		lic->cop_ier[i] = readl_relaxed(ictlr + ICTLR_COP_IER);
> >>>>   		lic->cop_iep[i] = readl_relaxed(ictlr + ICTLR_COP_IEP_CLASS);
> >>>>   
> >>>> -		/* Disable COP interrupts */
> >>>> -		writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
> >>>> +		/*
> >>>> +		 * AVP/COP/BPMP-Lite is the Tegra boot processor.
> >>>> +		 *
> >>>> +		 * Tegra210 system suspend flow uses sc7entry firmware which
> >>>> +		 * is executed by COP/BPMP and it includes disabling COP IRQ,
> >>>> +		 * clamping CPU rail, turning off VDD_CPU, and preparing the
> >>>> +		 * system to go to SC7/LP0.
> >>>> +		 *
> >>>> +		 * COP/BPMP wakes up when COP IRQ is triggered and runs
> >>>> +		 * sc7entry-firmware. So need to keep COP interrupt enabled.
> >>>> +		 */
> >>>> +		if (!lic->soc->supports_sc7)
> >>>> +			/* Disable COP interrupts if SC7 is not supported */  
> >>> All Tegra SoCs support SC7, hence the 'supports_sc7' and the comment
> >>> doesn't sound correct to me. Something like 'firmware_sc7' should suit
> >>> better here.  
> >> If what you're saying is true, then the whole patch is wrong, and the
> >> SC7 property should come from DT.  
> > It should be safe to assume that all of existing Tegra210 devices use
> > the firmware for SC7, hence I wouldn't say that the patch is entirely
> > wrong. To me it's not entirely correct.  
> 
> Yes, all existing Tegra210 platforms uses sc7 entry firmware for SC7 and 
> AVP/COP IRQ need to be kept enabled as during suspend ATF triggers IRQ 
> to COP for SC7 entry fw execution.

That's not the question. Dmitry says that the SC7 support is not a
property of the SoC, but mostly a platform decision on whether the
firmware supports SC7 or not.

To me, that's a clear indication that this should not be hardcoded in
the driver, but instead obtained dynamically, via DT or otherwise.

> 
> 
> >>>> +			writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);  
> >>> Secondly, I'm also not sure why COP interrupts need to be disabled for
> >>> pre-T210 at all, since COP is unused. This looks to me like it was
> >>> cut-n-pasted from downstream kernel without a good reason and could be
> >>> simply removed.  
> >> Please verify that this is actually the case. Tegra-2 definitely needed
> >> some level of poking, and I'm not keen on changing anything there until
> >> you (or someone else) has verified it on actual HW (see e307cc8941fc).  
> > Tested on Tegra20 and Tegra30, LP1 suspend-resume works perfectly fine
> > with all COP bits removed from the driver.
> >
> > AFAIK, the reason why downstream needed that disabling is that it uses
> > proprietary firmware which is running on the COP and that firmware is
> > usually a BLOB audio/video DEC-ENC driver which doesn't cleanup
> > interrupts after itself. That firmware is not applicable for the
> > upstream kernel, hence there is no need to care about it.
> >  
> >> Joseph, can you please shed some light here?  
> 
> SC7 entry flow uses 3rd party ATF (arm-trusted FW) blob which is the
> one that actually loads SC7 entry firmware and triggers IRQ to
> AVP/COP which causes COP to wakeup and run SC7 entry FW.
> 
> So when SC7 support is enabled, IRQ need to be kept enabled and when
> SC7 FW starts execution, it will disable COP IRQ.

This looks like a lot of undocumented assumptions on what firmware
does, as well as what firmware *is*. What I gather from this thread is
that there is at least two versions of firmware (a "proprietary
firmware" for "downstream kernels", and another one for mainline), and
that they do different things.

Given that we cannot know what people actually run, I don't think we
can safely remove anything unless this gets tested on the full spectrum
of HW/FW combination.

Thanks,

	M.
Dmitry Osipenko July 22, 2019, 11:35 p.m. UTC | #7
22.07.2019 21:38, Marc Zyngier пишет:
> On Mon, 22 Jul 2019 09:21:21 -0700
> Sowjanya Komatineni <skomatineni@nvidia.com> wrote:
> 
>> On 7/22/19 3:57 AM, Dmitry Osipenko wrote:
>>> 22.07.2019 13:13, Marc Zyngier пишет:  
>>>> On 22/07/2019 10:54, Dmitry Osipenko wrote:  
>>>>> 21.07.2019 22:40, Sowjanya Komatineni пишет:  
>>>>>> Tegra210 platforms use sc7 entry firmware to program Tegra LP0/SC7 entry
>>>>>> sequence and sc7 entry firmware is run from COP/BPMP-Lite.
>>>>>>
>>>>>> So, COP/BPMP-Lite still need IRQ function to finish SC7 suspend sequence
>>>>>> for Tegra210.
>>>>>>
>>>>>> This patch has fix for leaving the COP IRQ enabled for Tegra210 during
>>>>>> interrupt controller suspend operation.
>>>>>>
>>>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>> ---
>>>>>>   drivers/irqchip/irq-tegra.c | 20 ++++++++++++++++++--
>>>>>>   1 file changed, 18 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/irqchip/irq-tegra.c b/drivers/irqchip/irq-tegra.c
>>>>>> index e1f771c72fc4..851f88cef508 100644
>>>>>> --- a/drivers/irqchip/irq-tegra.c
>>>>>> +++ b/drivers/irqchip/irq-tegra.c
>>>>>> @@ -44,6 +44,7 @@ static unsigned int num_ictlrs;
>>>>>>   
>>>>>>   struct tegra_ictlr_soc {
>>>>>>   	unsigned int num_ictlrs;
>>>>>> +	bool supports_sc7;
>>>>>>   };
>>>>>>   
>>>>>>   static const struct tegra_ictlr_soc tegra20_ictlr_soc = {
>>>>>> @@ -56,6 +57,7 @@ static const struct tegra_ictlr_soc tegra30_ictlr_soc = {
>>>>>>   
>>>>>>   static const struct tegra_ictlr_soc tegra210_ictlr_soc = {
>>>>>>   	.num_ictlrs = 6,
>>>>>> +	.supports_sc7 = true,
>>>>>>   };
>>>>>>   
>>>>>>   static const struct of_device_id ictlr_matches[] = {
>>>>>> @@ -67,6 +69,7 @@ static const struct of_device_id ictlr_matches[] = {
>>>>>>   
>>>>>>   struct tegra_ictlr_info {
>>>>>>   	void __iomem *base[TEGRA_MAX_NUM_ICTLRS];
>>>>>> +	const struct tegra_ictlr_soc *soc;
>>>>>>   #ifdef CONFIG_PM_SLEEP
>>>>>>   	u32 cop_ier[TEGRA_MAX_NUM_ICTLRS];
>>>>>>   	u32 cop_iep[TEGRA_MAX_NUM_ICTLRS];
>>>>>> @@ -147,8 +150,20 @@ static int tegra_ictlr_suspend(void)
>>>>>>   		lic->cop_ier[i] = readl_relaxed(ictlr + ICTLR_COP_IER);
>>>>>>   		lic->cop_iep[i] = readl_relaxed(ictlr + ICTLR_COP_IEP_CLASS);
>>>>>>   
>>>>>> -		/* Disable COP interrupts */
>>>>>> -		writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
>>>>>> +		/*
>>>>>> +		 * AVP/COP/BPMP-Lite is the Tegra boot processor.
>>>>>> +		 *
>>>>>> +		 * Tegra210 system suspend flow uses sc7entry firmware which
>>>>>> +		 * is executed by COP/BPMP and it includes disabling COP IRQ,
>>>>>> +		 * clamping CPU rail, turning off VDD_CPU, and preparing the
>>>>>> +		 * system to go to SC7/LP0.
>>>>>> +		 *
>>>>>> +		 * COP/BPMP wakes up when COP IRQ is triggered and runs
>>>>>> +		 * sc7entry-firmware. So need to keep COP interrupt enabled.
>>>>>> +		 */
>>>>>> +		if (!lic->soc->supports_sc7)
>>>>>> +			/* Disable COP interrupts if SC7 is not supported */  
>>>>> All Tegra SoCs support SC7, hence the 'supports_sc7' and the comment
>>>>> doesn't sound correct to me. Something like 'firmware_sc7' should suit
>>>>> better here.  
>>>> If what you're saying is true, then the whole patch is wrong, and the
>>>> SC7 property should come from DT.  
>>> It should be safe to assume that all of existing Tegra210 devices use
>>> the firmware for SC7, hence I wouldn't say that the patch is entirely
>>> wrong. To me it's not entirely correct.  
>>
>> Yes, all existing Tegra210 platforms uses sc7 entry firmware for SC7 and 
>> AVP/COP IRQ need to be kept enabled as during suspend ATF triggers IRQ 
>> to COP for SC7 entry fw execution.

Okay, as I already wrote before, it looks to me that a more proper
solution should be to just remove everything related to COP from this
driver instead of adding custom quirks for T210.

The disabling / restoring of COP interrupts should be relevant only for
the multimedia firmware on older Tegra SoCs. That firmware won't be ever
supported in the upstream simply because NVIDIA abandoned the support
for older hardware in the downstream and because it is not possible due
to some legal weirdness (IIUC). The only variant for upstream is
reverse-engineering of hardware (not the firmware BLOB) and writing
proper opensource drivers for the upstream kernel, which we're already
doing and have success to a some extent.

> That's not the question. Dmitry says that the SC7 support is not a
> property of the SoC, but mostly a platform decision on whether the
> firmware supports SC7 or not.
> 
> To me, that's a clear indication that this should not be hardcoded in
> the driver, but instead obtained dynamically, via DT or otherwise.

We already have an nvidia,suspend-mode property in the device-tree of
the Power Management Controller node (all Tegra SoCs) which defines what
suspending type is supported by a particular board.

>>>>>> +			writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);  
>>>>> Secondly, I'm also not sure why COP interrupts need to be disabled for
>>>>> pre-T210 at all, since COP is unused. This looks to me like it was
>>>>> cut-n-pasted from downstream kernel without a good reason and could be
>>>>> simply removed.  
>>>> Please verify that this is actually the case. Tegra-2 definitely needed
>>>> some level of poking, and I'm not keen on changing anything there until
>>>> you (or someone else) has verified it on actual HW (see e307cc8941fc).  
>>> Tested on Tegra20 and Tegra30, LP1 suspend-resume works perfectly fine
>>> with all COP bits removed from the driver.
>>>
>>> AFAIK, the reason why downstream needed that disabling is that it uses
>>> proprietary firmware which is running on the COP and that firmware is
>>> usually a BLOB audio/video DEC-ENC driver which doesn't cleanup
>>> interrupts after itself. That firmware is not applicable for the
>>> upstream kernel, hence there is no need to care about it.
>>>  
>>>> Joseph, can you please shed some light here?  
>>
>> SC7 entry flow uses 3rd party ATF (arm-trusted FW) blob which is the
>> one that actually loads SC7 entry firmware and triggers IRQ to
>> AVP/COP which causes COP to wakeup and run SC7 entry FW.
>>
>> So when SC7 support is enabled, IRQ need to be kept enabled and when
>> SC7 FW starts execution, it will disable COP IRQ.
> 
> This looks like a lot of undocumented assumptions on what firmware
> does, as well as what firmware *is*. What I gather from this thread is
> that there is at least two versions of firmware (a "proprietary
> firmware" for "downstream kernels", and another one for mainline), and
> that they do different things.
> 
> Given that we cannot know what people actually run, I don't think we
> can safely remove anything unless this gets tested on the full spectrum
> of HW/FW combination.

I'm not sure whether multiple firmware variations exist in the wild for
Tegra210. Maybe Sowjanya or somebody else from NVIDIA could clarify. I
think there should be some efforts in regards to a fully opensource
firmware on Tegra210, but I'm not following it and have no idea about
the status.

You're right that there are multiple variants of suspend-resuming flow
on Tegra SoCs. The older 32bit Tegra SoC generations have a variety of
options in regards to suspend-resuming, including firmware-less variants
on platforms that are having kernel running in secure mode (dev boards,
most of Tegra20 consumer devices) and Trusted-Foundations firmware
variant for insecure platforms (consumer devices). And yes, vendor
firmware creates a lot of headache in regards to bringing support into
upstream because it usually does a lot of odd undocumented things which
may also vary depending on a firmware version (bootloader, etc) and it
also usually difficult to replace it with an opensource alternative due
to a crypto signing.
Sowjanya Komatineni July 24, 2019, 11:09 p.m. UTC | #8
On 7/22/19 4:35 PM, Dmitry Osipenko wrote:
> 22.07.2019 21:38, Marc Zyngier пишет:
>> On Mon, 22 Jul 2019 09:21:21 -0700
>> Sowjanya Komatineni <skomatineni@nvidia.com> wrote:
>>
>>> On 7/22/19 3:57 AM, Dmitry Osipenko wrote:
>>>> 22.07.2019 13:13, Marc Zyngier пишет:
>>>>> On 22/07/2019 10:54, Dmitry Osipenko wrote:
>>>>>> 21.07.2019 22:40, Sowjanya Komatineni пишет:
>>>>>>> Tegra210 platforms use sc7 entry firmware to program Tegra LP0/SC7 entry
>>>>>>> sequence and sc7 entry firmware is run from COP/BPMP-Lite.
>>>>>>>
>>>>>>> So, COP/BPMP-Lite still need IRQ function to finish SC7 suspend sequence
>>>>>>> for Tegra210.
>>>>>>>
>>>>>>> This patch has fix for leaving the COP IRQ enabled for Tegra210 during
>>>>>>> interrupt controller suspend operation.
>>>>>>>
>>>>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>> ---
>>>>>>>    drivers/irqchip/irq-tegra.c | 20 ++++++++++++++++++--
>>>>>>>    1 file changed, 18 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/irqchip/irq-tegra.c b/drivers/irqchip/irq-tegra.c
>>>>>>> index e1f771c72fc4..851f88cef508 100644
>>>>>>> --- a/drivers/irqchip/irq-tegra.c
>>>>>>> +++ b/drivers/irqchip/irq-tegra.c
>>>>>>> @@ -44,6 +44,7 @@ static unsigned int num_ictlrs;
>>>>>>>    
>>>>>>>    struct tegra_ictlr_soc {
>>>>>>>    	unsigned int num_ictlrs;
>>>>>>> +	bool supports_sc7;
>>>>>>>    };
>>>>>>>    
>>>>>>>    static const struct tegra_ictlr_soc tegra20_ictlr_soc = {
>>>>>>> @@ -56,6 +57,7 @@ static const struct tegra_ictlr_soc tegra30_ictlr_soc = {
>>>>>>>    
>>>>>>>    static const struct tegra_ictlr_soc tegra210_ictlr_soc = {
>>>>>>>    	.num_ictlrs = 6,
>>>>>>> +	.supports_sc7 = true,
>>>>>>>    };
>>>>>>>    
>>>>>>>    static const struct of_device_id ictlr_matches[] = {
>>>>>>> @@ -67,6 +69,7 @@ static const struct of_device_id ictlr_matches[] = {
>>>>>>>    
>>>>>>>    struct tegra_ictlr_info {
>>>>>>>    	void __iomem *base[TEGRA_MAX_NUM_ICTLRS];
>>>>>>> +	const struct tegra_ictlr_soc *soc;
>>>>>>>    #ifdef CONFIG_PM_SLEEP
>>>>>>>    	u32 cop_ier[TEGRA_MAX_NUM_ICTLRS];
>>>>>>>    	u32 cop_iep[TEGRA_MAX_NUM_ICTLRS];
>>>>>>> @@ -147,8 +150,20 @@ static int tegra_ictlr_suspend(void)
>>>>>>>    		lic->cop_ier[i] = readl_relaxed(ictlr + ICTLR_COP_IER);
>>>>>>>    		lic->cop_iep[i] = readl_relaxed(ictlr + ICTLR_COP_IEP_CLASS);
>>>>>>>    
>>>>>>> -		/* Disable COP interrupts */
>>>>>>> -		writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
>>>>>>> +		/*
>>>>>>> +		 * AVP/COP/BPMP-Lite is the Tegra boot processor.
>>>>>>> +		 *
>>>>>>> +		 * Tegra210 system suspend flow uses sc7entry firmware which
>>>>>>> +		 * is executed by COP/BPMP and it includes disabling COP IRQ,
>>>>>>> +		 * clamping CPU rail, turning off VDD_CPU, and preparing the
>>>>>>> +		 * system to go to SC7/LP0.
>>>>>>> +		 *
>>>>>>> +		 * COP/BPMP wakes up when COP IRQ is triggered and runs
>>>>>>> +		 * sc7entry-firmware. So need to keep COP interrupt enabled.
>>>>>>> +		 */
>>>>>>> +		if (!lic->soc->supports_sc7)
>>>>>>> +			/* Disable COP interrupts if SC7 is not supported */
>>>>>> All Tegra SoCs support SC7, hence the 'supports_sc7' and the comment
>>>>>> doesn't sound correct to me. Something like 'firmware_sc7' should suit
>>>>>> better here.
>>>>> If what you're saying is true, then the whole patch is wrong, and the
>>>>> SC7 property should come from DT.
>>>> It should be safe to assume that all of existing Tegra210 devices use
>>>> the firmware for SC7, hence I wouldn't say that the patch is entirely
>>>> wrong. To me it's not entirely correct.
>>> Yes, all existing Tegra210 platforms uses sc7 entry firmware for SC7 and
>>> AVP/COP IRQ need to be kept enabled as during suspend ATF triggers IRQ
>>> to COP for SC7 entry fw execution.
> Okay, as I already wrote before, it looks to me that a more proper
> solution should be to just remove everything related to COP from this
> driver instead of adding custom quirks for T210.
>
> The disabling / restoring of COP interrupts should be relevant only for
> the multimedia firmware on older Tegra SoCs. That firmware won't be ever
> supported in the upstream simply because NVIDIA abandoned the support
> for older hardware in the downstream and because it is not possible due
> to some legal weirdness (IIUC). The only variant for upstream is
> reverse-engineering of hardware (not the firmware BLOB) and writing
> proper opensource drivers for the upstream kernel, which we're already
> doing and have success to a some extent.
>
>> That's not the question. Dmitry says that the SC7 support is not a
>> property of the SoC, but mostly a platform decision on whether the
>> firmware supports SC7 or not.
>>
>> To me, that's a clear indication that this should not be hardcoded in
>> the driver, but instead obtained dynamically, via DT or otherwise.
> We already have an nvidia,suspend-mode property in the device-tree of
> the Power Management Controller node (all Tegra SoCs) which defines what
> suspending type is supported by a particular board.
>
>>>>>>> +			writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
>>>>>> Secondly, I'm also not sure why COP interrupts need to be disabled for
>>>>>> pre-T210 at all, since COP is unused. This looks to me like it was
>>>>>> cut-n-pasted from downstream kernel without a good reason and could be
>>>>>> simply removed.
>>>>> Please verify that this is actually the case. Tegra-2 definitely needed
>>>>> some level of poking, and I'm not keen on changing anything there until
>>>>> you (or someone else) has verified it on actual HW (see e307cc8941fc).
>>>> Tested on Tegra20 and Tegra30, LP1 suspend-resume works perfectly fine
>>>> with all COP bits removed from the driver.
>>>>
>>>> AFAIK, the reason why downstream needed that disabling is that it uses
>>>> proprietary firmware which is running on the COP and that firmware is
>>>> usually a BLOB audio/video DEC-ENC driver which doesn't cleanup
>>>> interrupts after itself. That firmware is not applicable for the
>>>> upstream kernel, hence there is no need to care about it.
>>>>   
>>>>> Joseph, can you please shed some light here?
>>> SC7 entry flow uses 3rd party ATF (arm-trusted FW) blob which is the
>>> one that actually loads SC7 entry firmware and triggers IRQ to
>>> AVP/COP which causes COP to wakeup and run SC7 entry FW.
>>>
>>> So when SC7 support is enabled, IRQ need to be kept enabled and when
>>> SC7 FW starts execution, it will disable COP IRQ.
>> This looks like a lot of undocumented assumptions on what firmware
>> does, as well as what firmware *is*. What I gather from this thread is
>> that there is at least two versions of firmware (a "proprietary
>> firmware" for "downstream kernels", and another one for mainline), and
>> that they do different things.
>>
>> Given that we cannot know what people actually run, I don't think we
>> can safely remove anything unless this gets tested on the full spectrum
>> of HW/FW combination.
> I'm not sure whether multiple firmware variations exist in the wild for
> Tegra210. Maybe Sowjanya or somebody else from NVIDIA could clarify. I
> think there should be some efforts in regards to a fully opensource
> firmware on Tegra210, but I'm not following it and have no idea about
> the status.
>
> You're right that there are multiple variants of suspend-resuming flow
> on Tegra SoCs. The older 32bit Tegra SoC generations have a variety of
> options in regards to suspend-resuming, including firmware-less variants
> on platforms that are having kernel running in secure mode (dev boards,
> most of Tegra20 consumer devices) and Trusted-Foundations firmware
> variant for insecure platforms (consumer devices). And yes, vendor
> firmware creates a lot of headache in regards to bringing support into
> upstream because it usually does a lot of odd undocumented things which
> may also vary depending on a firmware version (bootloader, etc) and it
> also usually difficult to replace it with an opensource alternative due
> to a crypto signing.

Tried without this patch which keeps COP IRQ disabled and I see SC7 
entry FW execution happens still.

Digging through the ATF FW code, I see on SC7 entry firmware loading 
into IRAM, COP processor is reset with RESET VECTOR set to SC7 entry 
firmware location in IRAM and on reset de-assert & unhalt COP, SC7 
firmware starts execution.

Will remove this patch in next version...
Peter De Schrijver July 25, 2019, 9:55 a.m. UTC | #9
On Mon, Jul 22, 2019 at 12:54:51PM +0300, Dmitry Osipenko wrote:
> 
> All Tegra SoCs support SC7, hence the 'supports_sc7' and the comment
> doesn't sound correct to me. Something like 'firmware_sc7' should suit
> better here.
> 
> > +			writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
> 
> Secondly, I'm also not sure why COP interrupts need to be disabled for
> pre-T210 at all, since COP is unused. This looks to me like it was
> cut-n-pasted from downstream kernel without a good reason and could be
> simply removed.

I don't think we can rely on the fact that COP is unused. People can
write their own code to run on COP.

Peter.
Dmitry Osipenko July 25, 2019, 10:05 a.m. UTC | #10
25.07.2019 12:55, Peter De Schrijver пишет:
> On Mon, Jul 22, 2019 at 12:54:51PM +0300, Dmitry Osipenko wrote:
>>
>> All Tegra SoCs support SC7, hence the 'supports_sc7' and the comment
>> doesn't sound correct to me. Something like 'firmware_sc7' should suit
>> better here.
>>
>>> +			writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
>>
>> Secondly, I'm also not sure why COP interrupts need to be disabled for
>> pre-T210 at all, since COP is unused. This looks to me like it was
>> cut-n-pasted from downstream kernel without a good reason and could be
>> simply removed.
> 
> I don't think we can rely on the fact that COP is unused. People can
> write their own code to run on COP.

1. Not upstream - doesn't matter.

2. That's not very good if something unknown is running on COP and then
kernel suddenly intervenes, don't you think so?
Peter De Schrijver July 25, 2019, 10:33 a.m. UTC | #11
On Thu, Jul 25, 2019 at 01:05:13PM +0300, Dmitry Osipenko wrote:
> 25.07.2019 12:55, Peter De Schrijver пишет:
> > On Mon, Jul 22, 2019 at 12:54:51PM +0300, Dmitry Osipenko wrote:
> >>
> >> All Tegra SoCs support SC7, hence the 'supports_sc7' and the comment
> >> doesn't sound correct to me. Something like 'firmware_sc7' should suit
> >> better here.
> >>
> >>> +			writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
> >>
> >> Secondly, I'm also not sure why COP interrupts need to be disabled for
> >> pre-T210 at all, since COP is unused. This looks to me like it was
> >> cut-n-pasted from downstream kernel without a good reason and could be
> >> simply removed.
> > 
> > I don't think we can rely on the fact that COP is unused. People can
> > write their own code to run on COP.
> 
> 1. Not upstream - doesn't matter.
> 

The code is not part of the kernel, so obviously it's not upstream?

> 2. That's not very good if something unknown is running on COP and then
> kernel suddenly intervenes, don't you think so?

Unless the code was written with this in mind.

Peter.
Peter De Schrijver July 25, 2019, 10:38 a.m. UTC | #12
On Thu, Jul 25, 2019 at 01:33:48PM +0300, Peter De Schrijver wrote:
> On Thu, Jul 25, 2019 at 01:05:13PM +0300, Dmitry Osipenko wrote:
> > 25.07.2019 12:55, Peter De Schrijver пишет:
> > > On Mon, Jul 22, 2019 at 12:54:51PM +0300, Dmitry Osipenko wrote:
> > >>
> > >> All Tegra SoCs support SC7, hence the 'supports_sc7' and the comment
> > >> doesn't sound correct to me. Something like 'firmware_sc7' should suit
> > >> better here.
> > >>
> > >>> +			writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
> > >>
> > >> Secondly, I'm also not sure why COP interrupts need to be disabled for
> > >> pre-T210 at all, since COP is unused. This looks to me like it was
> > >> cut-n-pasted from downstream kernel without a good reason and could be
> > >> simply removed.
> > > 
> > > I don't think we can rely on the fact that COP is unused. People can
> > > write their own code to run on COP.
> > 
> > 1. Not upstream - doesn't matter.
> > 
> 
> The code is not part of the kernel, so obviously it's not upstream?
> 
> > 2. That's not very good if something unknown is running on COP and then
> > kernel suddenly intervenes, don't you think so?
> 
> Unless the code was written with this in mind.
> 

Looking at this again, I don't think we need to enable the IRQ at all.

Peter.
Dmitry Osipenko July 25, 2019, 10:59 a.m. UTC | #13
25.07.2019 13:38, Peter De Schrijver пишет:
> On Thu, Jul 25, 2019 at 01:33:48PM +0300, Peter De Schrijver wrote:
>> On Thu, Jul 25, 2019 at 01:05:13PM +0300, Dmitry Osipenko wrote:
>>> 25.07.2019 12:55, Peter De Schrijver пишет:
>>>> On Mon, Jul 22, 2019 at 12:54:51PM +0300, Dmitry Osipenko wrote:
>>>>>
>>>>> All Tegra SoCs support SC7, hence the 'supports_sc7' and the comment
>>>>> doesn't sound correct to me. Something like 'firmware_sc7' should suit
>>>>> better here.
>>>>>
>>>>>> +			writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
>>>>>
>>>>> Secondly, I'm also not sure why COP interrupts need to be disabled for
>>>>> pre-T210 at all, since COP is unused. This looks to me like it was
>>>>> cut-n-pasted from downstream kernel without a good reason and could be
>>>>> simply removed.
>>>>
>>>> I don't think we can rely on the fact that COP is unused. People can
>>>> write their own code to run on COP.
>>>
>>> 1. Not upstream - doesn't matter.
>>>
>>
>> The code is not part of the kernel, so obviously it's not upstream?
>>
>>> 2. That's not very good if something unknown is running on COP and then
>>> kernel suddenly intervenes, don't you think so?
>>
>> Unless the code was written with this in mind.
>>

In that case, please see 1. ;)

> 
> Looking at this again, I don't think we need to enable the IRQ at all.

Could you please clarify? The code only saves/restores COP's interrupts
context across suspend-resume.

Again, that's absolutely useless code for the upstream kernel which
could be removed safely to avoid the confusion, IMHO. I can type a patch
if you're agreeing.
Dmitry Osipenko July 26, 2019, 4:48 a.m. UTC | #14
В Wed, 24 Jul 2019 16:09:53 -0700
Sowjanya Komatineni <skomatineni@nvidia.com> пишет:

> On 7/22/19 4:35 PM, Dmitry Osipenko wrote:
> > 22.07.2019 21:38, Marc Zyngier пишет:  
> >> On Mon, 22 Jul 2019 09:21:21 -0700
> >> Sowjanya Komatineni <skomatineni@nvidia.com> wrote:
> >>  
> >>> On 7/22/19 3:57 AM, Dmitry Osipenko wrote:  
> >>>> 22.07.2019 13:13, Marc Zyngier пишет:  
> >>>>> On 22/07/2019 10:54, Dmitry Osipenko wrote:  
> >>>>>> 21.07.2019 22:40, Sowjanya Komatineni пишет:  
> >>>>>>> Tegra210 platforms use sc7 entry firmware to program Tegra
> >>>>>>> LP0/SC7 entry sequence and sc7 entry firmware is run from
> >>>>>>> COP/BPMP-Lite.
> >>>>>>>
> >>>>>>> So, COP/BPMP-Lite still need IRQ function to finish SC7
> >>>>>>> suspend sequence for Tegra210.
> >>>>>>>
> >>>>>>> This patch has fix for leaving the COP IRQ enabled for
> >>>>>>> Tegra210 during interrupt controller suspend operation.
> >>>>>>>
> >>>>>>> Acked-by: Thierry Reding <treding@nvidia.com>
> >>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> >>>>>>> ---
> >>>>>>>    drivers/irqchip/irq-tegra.c | 20 ++++++++++++++++++--
> >>>>>>>    1 file changed, 18 insertions(+), 2 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/irqchip/irq-tegra.c
> >>>>>>> b/drivers/irqchip/irq-tegra.c index
> >>>>>>> e1f771c72fc4..851f88cef508 100644 ---
> >>>>>>> a/drivers/irqchip/irq-tegra.c +++
> >>>>>>> b/drivers/irqchip/irq-tegra.c @@ -44,6 +44,7 @@ static
> >>>>>>> unsigned int num_ictlrs; 
> >>>>>>>    struct tegra_ictlr_soc {
> >>>>>>>    	unsigned int num_ictlrs;
> >>>>>>> +	bool supports_sc7;
> >>>>>>>    };
> >>>>>>>    
> >>>>>>>    static const struct tegra_ictlr_soc tegra20_ictlr_soc = {
> >>>>>>> @@ -56,6 +57,7 @@ static const struct tegra_ictlr_soc
> >>>>>>> tegra30_ictlr_soc = { 
> >>>>>>>    static const struct tegra_ictlr_soc tegra210_ictlr_soc = {
> >>>>>>>    	.num_ictlrs = 6,
> >>>>>>> +	.supports_sc7 = true,
> >>>>>>>    };
> >>>>>>>    
> >>>>>>>    static const struct of_device_id ictlr_matches[] = {
> >>>>>>> @@ -67,6 +69,7 @@ static const struct of_device_id
> >>>>>>> ictlr_matches[] = { 
> >>>>>>>    struct tegra_ictlr_info {
> >>>>>>>    	void __iomem *base[TEGRA_MAX_NUM_ICTLRS];
> >>>>>>> +	const struct tegra_ictlr_soc *soc;
> >>>>>>>    #ifdef CONFIG_PM_SLEEP
> >>>>>>>    	u32 cop_ier[TEGRA_MAX_NUM_ICTLRS];
> >>>>>>>    	u32 cop_iep[TEGRA_MAX_NUM_ICTLRS];
> >>>>>>> @@ -147,8 +150,20 @@ static int tegra_ictlr_suspend(void)
> >>>>>>>    		lic->cop_ier[i] = readl_relaxed(ictlr +
> >>>>>>> ICTLR_COP_IER); lic->cop_iep[i] = readl_relaxed(ictlr +
> >>>>>>> ICTLR_COP_IEP_CLASS); 
> >>>>>>> -		/* Disable COP interrupts */
> >>>>>>> -		writel_relaxed(~0ul, ictlr +
> >>>>>>> ICTLR_COP_IER_CLR);
> >>>>>>> +		/*
> >>>>>>> +		 * AVP/COP/BPMP-Lite is the Tegra boot
> >>>>>>> processor.
> >>>>>>> +		 *
> >>>>>>> +		 * Tegra210 system suspend flow uses
> >>>>>>> sc7entry firmware which
> >>>>>>> +		 * is executed by COP/BPMP and it includes
> >>>>>>> disabling COP IRQ,
> >>>>>>> +		 * clamping CPU rail, turning off VDD_CPU,
> >>>>>>> and preparing the
> >>>>>>> +		 * system to go to SC7/LP0.
> >>>>>>> +		 *
> >>>>>>> +		 * COP/BPMP wakes up when COP IRQ is
> >>>>>>> triggered and runs
> >>>>>>> +		 * sc7entry-firmware. So need to keep COP
> >>>>>>> interrupt enabled.
> >>>>>>> +		 */
> >>>>>>> +		if (!lic->soc->supports_sc7)
> >>>>>>> +			/* Disable COP interrupts if SC7 is
> >>>>>>> not supported */  
> >>>>>> All Tegra SoCs support SC7, hence the 'supports_sc7' and the
> >>>>>> comment doesn't sound correct to me. Something like
> >>>>>> 'firmware_sc7' should suit better here.  
> >>>>> If what you're saying is true, then the whole patch is wrong,
> >>>>> and the SC7 property should come from DT.  
> >>>> It should be safe to assume that all of existing Tegra210
> >>>> devices use the firmware for SC7, hence I wouldn't say that the
> >>>> patch is entirely wrong. To me it's not entirely correct.  
> >>> Yes, all existing Tegra210 platforms uses sc7 entry firmware for
> >>> SC7 and AVP/COP IRQ need to be kept enabled as during suspend ATF
> >>> triggers IRQ to COP for SC7 entry fw execution.  
> > Okay, as I already wrote before, it looks to me that a more proper
> > solution should be to just remove everything related to COP from
> > this driver instead of adding custom quirks for T210.
> >
> > The disabling / restoring of COP interrupts should be relevant only
> > for the multimedia firmware on older Tegra SoCs. That firmware
> > won't be ever supported in the upstream simply because NVIDIA
> > abandoned the support for older hardware in the downstream and
> > because it is not possible due to some legal weirdness (IIUC). The
> > only variant for upstream is reverse-engineering of hardware (not
> > the firmware BLOB) and writing proper opensource drivers for the
> > upstream kernel, which we're already doing and have success to a
> > some extent. 
> >> That's not the question. Dmitry says that the SC7 support is not a
> >> property of the SoC, but mostly a platform decision on whether the
> >> firmware supports SC7 or not.
> >>
> >> To me, that's a clear indication that this should not be hardcoded
> >> in the driver, but instead obtained dynamically, via DT or
> >> otherwise.  
> > We already have an nvidia,suspend-mode property in the device-tree
> > of the Power Management Controller node (all Tegra SoCs) which
> > defines what suspending type is supported by a particular board.
> >  
> >>>>>>> +			writel_relaxed(~0ul, ictlr +
> >>>>>>> ICTLR_COP_IER_CLR);  
> >>>>>> Secondly, I'm also not sure why COP interrupts need to be
> >>>>>> disabled for pre-T210 at all, since COP is unused. This looks
> >>>>>> to me like it was cut-n-pasted from downstream kernel without
> >>>>>> a good reason and could be simply removed.  
> >>>>> Please verify that this is actually the case. Tegra-2
> >>>>> definitely needed some level of poking, and I'm not keen on
> >>>>> changing anything there until you (or someone else) has
> >>>>> verified it on actual HW (see e307cc8941fc).  
> >>>> Tested on Tegra20 and Tegra30, LP1 suspend-resume works
> >>>> perfectly fine with all COP bits removed from the driver.
> >>>>
> >>>> AFAIK, the reason why downstream needed that disabling is that
> >>>> it uses proprietary firmware which is running on the COP and
> >>>> that firmware is usually a BLOB audio/video DEC-ENC driver which
> >>>> doesn't cleanup interrupts after itself. That firmware is not
> >>>> applicable for the upstream kernel, hence there is no need to
> >>>> care about it. 
> >>>>> Joseph, can you please shed some light here?  
> >>> SC7 entry flow uses 3rd party ATF (arm-trusted FW) blob which is
> >>> the one that actually loads SC7 entry firmware and triggers IRQ to
> >>> AVP/COP which causes COP to wakeup and run SC7 entry FW.
> >>>
> >>> So when SC7 support is enabled, IRQ need to be kept enabled and
> >>> when SC7 FW starts execution, it will disable COP IRQ.  
> >> This looks like a lot of undocumented assumptions on what firmware
> >> does, as well as what firmware *is*. What I gather from this
> >> thread is that there is at least two versions of firmware (a
> >> "proprietary firmware" for "downstream kernels", and another one
> >> for mainline), and that they do different things.
> >>
> >> Given that we cannot know what people actually run, I don't think
> >> we can safely remove anything unless this gets tested on the full
> >> spectrum of HW/FW combination.  
> > I'm not sure whether multiple firmware variations exist in the wild
> > for Tegra210. Maybe Sowjanya or somebody else from NVIDIA could
> > clarify. I think there should be some efforts in regards to a fully
> > opensource firmware on Tegra210, but I'm not following it and have
> > no idea about the status.
> >
> > You're right that there are multiple variants of suspend-resuming
> > flow on Tegra SoCs. The older 32bit Tegra SoC generations have a
> > variety of options in regards to suspend-resuming, including
> > firmware-less variants on platforms that are having kernel running
> > in secure mode (dev boards, most of Tegra20 consumer devices) and
> > Trusted-Foundations firmware variant for insecure platforms
> > (consumer devices). And yes, vendor firmware creates a lot of
> > headache in regards to bringing support into upstream because it
> > usually does a lot of odd undocumented things which may also vary
> > depending on a firmware version (bootloader, etc) and it also
> > usually difficult to replace it with an opensource alternative due
> > to a crypto signing.  
> 
> Tried without this patch which keeps COP IRQ disabled and I see SC7 
> entry FW execution happens still.
> 
> Digging through the ATF FW code, I see on SC7 entry firmware loading 
> into IRAM, COP processor is reset with RESET VECTOR set to SC7 entry 
> firmware location in IRAM and on reset de-assert & unhalt COP, SC7 
> firmware starts execution.
> 
> Will remove this patch in next version...
> 

Good, sounds like you also verified that SC7 COP firmware doesn't use
interrupts.
Peter De Schrijver Aug. 2, 2019, 1:05 p.m. UTC | #15
On Thu, Jul 25, 2019 at 01:59:09PM +0300, Dmitry Osipenko wrote:
> 25.07.2019 13:38, Peter De Schrijver пишет:
> > On Thu, Jul 25, 2019 at 01:33:48PM +0300, Peter De Schrijver wrote:
> >> On Thu, Jul 25, 2019 at 01:05:13PM +0300, Dmitry Osipenko wrote:
> >>> 25.07.2019 12:55, Peter De Schrijver пишет:
> >>>> On Mon, Jul 22, 2019 at 12:54:51PM +0300, Dmitry Osipenko wrote:
> >>>>>
> >>>>> All Tegra SoCs support SC7, hence the 'supports_sc7' and the comment
> >>>>> doesn't sound correct to me. Something like 'firmware_sc7' should suit
> >>>>> better here.
> >>>>>
> >>>>>> +			writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
> >>>>>
> >>>>> Secondly, I'm also not sure why COP interrupts need to be disabled for
> >>>>> pre-T210 at all, since COP is unused. This looks to me like it was
> >>>>> cut-n-pasted from downstream kernel without a good reason and could be
> >>>>> simply removed.
> >>>>
> >>>> I don't think we can rely on the fact that COP is unused. People can
> >>>> write their own code to run on COP.
> >>>
> >>> 1. Not upstream - doesn't matter.
> >>>
> >>
> >> The code is not part of the kernel, so obviously it's not upstream?
> >>
> >>> 2. That's not very good if something unknown is running on COP and then
> >>> kernel suddenly intervenes, don't you think so?
> >>
> >> Unless the code was written with this in mind.
> >>
> 
> In that case, please see 1. ;)
> 

In general the kernel should not touch the COP interrupts I think.

> > 
> > Looking at this again, I don't think we need to enable the IRQ at all.
> 
> Could you please clarify? The code only saves/restores COP's interrupts
> context across suspend-resume.

The sc7 entry firmware doesn't use interrupts.

Peter.
Dmitry Osipenko Aug. 2, 2019, 5:35 p.m. UTC | #16
02.08.2019 16:05, Peter De Schrijver пишет:
> On Thu, Jul 25, 2019 at 01:59:09PM +0300, Dmitry Osipenko wrote:
>> 25.07.2019 13:38, Peter De Schrijver пишет:
>>> On Thu, Jul 25, 2019 at 01:33:48PM +0300, Peter De Schrijver wrote:
>>>> On Thu, Jul 25, 2019 at 01:05:13PM +0300, Dmitry Osipenko wrote:
>>>>> 25.07.2019 12:55, Peter De Schrijver пишет:
>>>>>> On Mon, Jul 22, 2019 at 12:54:51PM +0300, Dmitry Osipenko wrote:
>>>>>>>
>>>>>>> All Tegra SoCs support SC7, hence the 'supports_sc7' and the comment
>>>>>>> doesn't sound correct to me. Something like 'firmware_sc7' should suit
>>>>>>> better here.
>>>>>>>
>>>>>>>> +			writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
>>>>>>>
>>>>>>> Secondly, I'm also not sure why COP interrupts need to be disabled for
>>>>>>> pre-T210 at all, since COP is unused. This looks to me like it was
>>>>>>> cut-n-pasted from downstream kernel without a good reason and could be
>>>>>>> simply removed.
>>>>>>
>>>>>> I don't think we can rely on the fact that COP is unused. People can
>>>>>> write their own code to run on COP.
>>>>>
>>>>> 1. Not upstream - doesn't matter.
>>>>>
>>>>
>>>> The code is not part of the kernel, so obviously it's not upstream?
>>>>
>>>>> 2. That's not very good if something unknown is running on COP and then
>>>>> kernel suddenly intervenes, don't you think so?
>>>>
>>>> Unless the code was written with this in mind.
>>>>
>>
>> In that case, please see 1. ;)
>>
> 
> In general the kernel should not touch the COP interrupts I think.
> 
>>>
>>> Looking at this again, I don't think we need to enable the IRQ at all.
>>
>> Could you please clarify? The code only saves/restores COP's interrupts
>> context across suspend-resume.
> 
> The sc7 entry firmware doesn't use interrupts.

Okay, it shouldn't hurt to clean up the LIC's code a tad by removing the
COP's bits, will make a patch.

Patch
diff mbox series

diff --git a/drivers/irqchip/irq-tegra.c b/drivers/irqchip/irq-tegra.c
index e1f771c72fc4..851f88cef508 100644
--- a/drivers/irqchip/irq-tegra.c
+++ b/drivers/irqchip/irq-tegra.c
@@ -44,6 +44,7 @@  static unsigned int num_ictlrs;
 
 struct tegra_ictlr_soc {
 	unsigned int num_ictlrs;
+	bool supports_sc7;
 };
 
 static const struct tegra_ictlr_soc tegra20_ictlr_soc = {
@@ -56,6 +57,7 @@  static const struct tegra_ictlr_soc tegra30_ictlr_soc = {
 
 static const struct tegra_ictlr_soc tegra210_ictlr_soc = {
 	.num_ictlrs = 6,
+	.supports_sc7 = true,
 };
 
 static const struct of_device_id ictlr_matches[] = {
@@ -67,6 +69,7 @@  static const struct of_device_id ictlr_matches[] = {
 
 struct tegra_ictlr_info {
 	void __iomem *base[TEGRA_MAX_NUM_ICTLRS];
+	const struct tegra_ictlr_soc *soc;
 #ifdef CONFIG_PM_SLEEP
 	u32 cop_ier[TEGRA_MAX_NUM_ICTLRS];
 	u32 cop_iep[TEGRA_MAX_NUM_ICTLRS];
@@ -147,8 +150,20 @@  static int tegra_ictlr_suspend(void)
 		lic->cop_ier[i] = readl_relaxed(ictlr + ICTLR_COP_IER);
 		lic->cop_iep[i] = readl_relaxed(ictlr + ICTLR_COP_IEP_CLASS);
 
-		/* Disable COP interrupts */
-		writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
+		/*
+		 * AVP/COP/BPMP-Lite is the Tegra boot processor.
+		 *
+		 * Tegra210 system suspend flow uses sc7entry firmware which
+		 * is executed by COP/BPMP and it includes disabling COP IRQ,
+		 * clamping CPU rail, turning off VDD_CPU, and preparing the
+		 * system to go to SC7/LP0.
+		 *
+		 * COP/BPMP wakes up when COP IRQ is triggered and runs
+		 * sc7entry-firmware. So need to keep COP interrupt enabled.
+		 */
+		if (!lic->soc->supports_sc7)
+			/* Disable COP interrupts if SC7 is not supported */
+			writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
 
 		/* Disable CPU interrupts */
 		writel_relaxed(~0ul, ictlr + ICTLR_CPU_IER_CLR);
@@ -339,6 +354,7 @@  static int __init tegra_ictlr_init(struct device_node *node,
 		goto out_unmap;
 	}
 
+	lic->soc = soc;
 	tegra_ictlr_syscore_init();
 
 	pr_info("%pOF: %d interrupts forwarded to %pOF\n",