diff mbox series

[V6,16/21] soc/tegra: pmc: Add pmc wake support for tegra210

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

Commit Message

Sowjanya Komatineni July 21, 2019, 7:40 p.m. UTC
This patch implements PMC wakeup sequence for Tegra210 and defines
common used RTC alarm wake event.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 drivers/soc/tegra/pmc.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 111 insertions(+)

Comments

Dmitry Osipenko July 23, 2019, 12:58 a.m. UTC | #1
21.07.2019 22:40, Sowjanya Komatineni пишет:
> This patch implements PMC wakeup sequence for Tegra210 and defines
> common used RTC alarm wake event.
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  drivers/soc/tegra/pmc.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 111 insertions(+)
> 
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index 91c84d0e66ae..c556f38874e1 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -57,6 +57,12 @@
>  #define  PMC_CNTRL_SYSCLK_OE		BIT(11) /* system clock enable */
>  #define  PMC_CNTRL_SYSCLK_POLARITY	BIT(10) /* sys clk polarity */
>  #define  PMC_CNTRL_MAIN_RST		BIT(4)
> +#define  PMC_CNTRL_LATCH_WAKEUPS	BIT(5)
> +
> +#define PMC_WAKE_MASK			0x0c
> +#define PMC_WAKE_LEVEL			0x10
> +#define PMC_WAKE_STATUS			0x14
> +#define PMC_SW_WAKE_STATUS		0x18
>  
>  #define DPD_SAMPLE			0x020
>  #define  DPD_SAMPLE_ENABLE		BIT(0)
> @@ -87,6 +93,11 @@
>  
>  #define PMC_SCRATCH41			0x140
>  
> +#define PMC_WAKE2_MASK			0x160
> +#define PMC_WAKE2_LEVEL			0x164
> +#define PMC_WAKE2_STATUS		0x168
> +#define PMC_SW_WAKE2_STATUS		0x16c
> +
>  #define PMC_SENSOR_CTRL			0x1b0
>  #define  PMC_SENSOR_CTRL_SCRATCH_WRITE	BIT(2)
>  #define  PMC_SENSOR_CTRL_ENABLE_RST	BIT(1)
> @@ -1922,6 +1933,55 @@ static const struct irq_domain_ops tegra_pmc_irq_domain_ops = {
>  	.alloc = tegra_pmc_irq_alloc,
>  };
>  
> +static int tegra210_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
> +{
> +	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
> +	unsigned int offset, bit;
> +	u32 value;
> +
> +	if (data->hwirq == ULONG_MAX)
> +		return 0;
> +
> +	offset = data->hwirq / 32;
> +	bit = data->hwirq % 32;
> +
> +	/*
> +	 * Latch wakeups to SW_WAKE_STATUS register to capture events
> +	 * that would not make it into wakeup event register during LP0 exit.
> +	 */
> +	value = tegra_pmc_readl(pmc, PMC_CNTRL);
> +	value |= PMC_CNTRL_LATCH_WAKEUPS;
> +	tegra_pmc_writel(pmc, value, PMC_CNTRL);
> +	udelay(120);

Why it takes so much time to latch the values? Shouldn't some status-bit
be polled for the completion of latching?

Is this register-write really getting buffered in the PMC?

> +	value &= ~PMC_CNTRL_LATCH_WAKEUPS;
> +	tegra_pmc_writel(pmc, value, PMC_CNTRL);
> +	udelay(120);

120 usecs to remove latching, really?

> +	tegra_pmc_writel(pmc, 0, PMC_SW_WAKE_STATUS);
> +	tegra_pmc_writel(pmc, 0, PMC_SW_WAKE2_STATUS);
> +
> +	tegra_pmc_writel(pmc, 0, PMC_WAKE_STATUS);
> +	tegra_pmc_writel(pmc, 0, PMC_WAKE2_STATUS);
> +
> +	/* enable PMC wake */
> +	if (data->hwirq >= 32)
> +		offset = PMC_WAKE2_MASK;
> +	else
> +		offset = PMC_WAKE_MASK;
> +
> +	value = tegra_pmc_readl(pmc, offset);
> +
> +	if (on)
> +		value |= 1 << bit;
> +	else
> +		value &= ~(1 << bit);
> +
> +	tegra_pmc_writel(pmc, value, offset);

Why the latching is done *before* writing into the WAKE registers? What
it is latching then?

> +	return 0;
> +}
> +
>  static int tegra186_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
>  {
>  	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
> @@ -1954,6 +2014,49 @@ static int tegra186_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
>  	return 0;
>  }
>  
> +static int tegra210_pmc_irq_set_type(struct irq_data *data, unsigned int type)
> +{
> +	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
> +	unsigned int offset, bit;
> +	u32 value;
> +
> +	if (data->hwirq == ULONG_MAX)
> +		return 0;
> +
> +	offset = data->hwirq / 32;
> +	bit = data->hwirq % 32;
> +
> +	if (data->hwirq >= 32)
> +		offset = PMC_WAKE2_LEVEL;
> +	else
> +		offset = PMC_WAKE_LEVEL;
> +
> +	value = tegra_pmc_readl(pmc, offset);
> +
> +	switch (type) {
> +	case IRQ_TYPE_EDGE_RISING:
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		value |= 1 << bit;
> +		break;
> +
> +	case IRQ_TYPE_EDGE_FALLING:
> +	case IRQ_TYPE_LEVEL_LOW:
> +		value &= ~(1 << bit);
> +		break;
> +
> +	case IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING:
> +		value ^= 1 << bit;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	tegra_pmc_writel(pmc, value, offset);

Shouldn't the WAKE_LEVEL be latched as well?

> +	return 0;
> +}
> +
>  static int tegra186_pmc_irq_set_type(struct irq_data *data, unsigned int type)
>  {
>  	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
> @@ -2540,6 +2643,10 @@ static const struct pinctrl_pin_desc tegra210_pin_descs[] = {
>  	TEGRA210_IO_PAD_TABLE(TEGRA_IO_PIN_DESC)
>  };
>  
> +static const struct tegra_wake_event tegra210_wake_events[] = {
> +	TEGRA_WAKE_IRQ("rtc", 16, 2),
> +};
> +
>  static const struct tegra_pmc_soc tegra210_pmc_soc = {
>  	.num_powergates = ARRAY_SIZE(tegra210_powergates),
>  	.powergates = tegra210_powergates,
> @@ -2557,10 +2664,14 @@ static const struct tegra_pmc_soc tegra210_pmc_soc = {
>  	.regs = &tegra20_pmc_regs,
>  	.init = tegra20_pmc_init,
>  	.setup_irq_polarity = tegra20_pmc_setup_irq_polarity,
> +	.irq_set_wake = tegra210_pmc_irq_set_wake,
> +	.irq_set_type = tegra210_pmc_irq_set_type,
>  	.reset_sources = tegra210_reset_sources,
>  	.num_reset_sources = ARRAY_SIZE(tegra210_reset_sources),
>  	.reset_levels = NULL,
>  	.num_reset_levels = 0,
> +	.num_wake_events = ARRAY_SIZE(tegra210_wake_events),
> +	.wake_events = tegra210_wake_events,
>  };
>  
>  #define TEGRA186_IO_PAD_TABLE(_pad)					     \
>
Dmitry Osipenko July 23, 2019, 1:08 a.m. UTC | #2
23.07.2019 3:58, Dmitry Osipenko пишет:
> 21.07.2019 22:40, Sowjanya Komatineni пишет:
>> This patch implements PMC wakeup sequence for Tegra210 and defines
>> common used RTC alarm wake event.
>>
>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>> ---
>>  drivers/soc/tegra/pmc.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 111 insertions(+)
>>
>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>> index 91c84d0e66ae..c556f38874e1 100644
>> --- a/drivers/soc/tegra/pmc.c
>> +++ b/drivers/soc/tegra/pmc.c
>> @@ -57,6 +57,12 @@
>>  #define  PMC_CNTRL_SYSCLK_OE		BIT(11) /* system clock enable */
>>  #define  PMC_CNTRL_SYSCLK_POLARITY	BIT(10) /* sys clk polarity */
>>  #define  PMC_CNTRL_MAIN_RST		BIT(4)
>> +#define  PMC_CNTRL_LATCH_WAKEUPS	BIT(5)

Please follow the TRM's bits naming.

PMC_CNTRL_LATCHWAKE_EN

>> +#define PMC_WAKE_MASK			0x0c
>> +#define PMC_WAKE_LEVEL			0x10
>> +#define PMC_WAKE_STATUS			0x14
>> +#define PMC_SW_WAKE_STATUS		0x18
>>  
>>  #define DPD_SAMPLE			0x020
>>  #define  DPD_SAMPLE_ENABLE		BIT(0)
>> @@ -87,6 +93,11 @@
>>  
>>  #define PMC_SCRATCH41			0x140
>>  
>> +#define PMC_WAKE2_MASK			0x160
>> +#define PMC_WAKE2_LEVEL			0x164
>> +#define PMC_WAKE2_STATUS		0x168
>> +#define PMC_SW_WAKE2_STATUS		0x16c
>> +
>>  #define PMC_SENSOR_CTRL			0x1b0
>>  #define  PMC_SENSOR_CTRL_SCRATCH_WRITE	BIT(2)
>>  #define  PMC_SENSOR_CTRL_ENABLE_RST	BIT(1)
>> @@ -1922,6 +1933,55 @@ static const struct irq_domain_ops tegra_pmc_irq_domain_ops = {
>>  	.alloc = tegra_pmc_irq_alloc,
>>  };
>>  
>> +static int tegra210_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
>> +{
>> +	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>> +	unsigned int offset, bit;
>> +	u32 value;
>> +
>> +	if (data->hwirq == ULONG_MAX)
>> +		return 0;
>> +
>> +	offset = data->hwirq / 32;
>> +	bit = data->hwirq % 32;
>> +
>> +	/*
>> +	 * Latch wakeups to SW_WAKE_STATUS register to capture events
>> +	 * that would not make it into wakeup event register during LP0 exit.
>> +	 */
>> +	value = tegra_pmc_readl(pmc, PMC_CNTRL);
>> +	value |= PMC_CNTRL_LATCH_WAKEUPS;
>> +	tegra_pmc_writel(pmc, value, PMC_CNTRL);
>> +	udelay(120);
> 
> Why it takes so much time to latch the values? Shouldn't some status-bit
> be polled for the completion of latching?
> 
> Is this register-write really getting buffered in the PMC?
> 
>> +	value &= ~PMC_CNTRL_LATCH_WAKEUPS;
>> +	tegra_pmc_writel(pmc, value, PMC_CNTRL);
>> +	udelay(120);
> 
> 120 usecs to remove latching, really?
> 
>> +	tegra_pmc_writel(pmc, 0, PMC_SW_WAKE_STATUS);
>> +	tegra_pmc_writel(pmc, 0, PMC_SW_WAKE2_STATUS);
>> +
>> +	tegra_pmc_writel(pmc, 0, PMC_WAKE_STATUS);
>> +	tegra_pmc_writel(pmc, 0, PMC_WAKE2_STATUS);
>> +
>> +	/* enable PMC wake */
>> +	if (data->hwirq >= 32)
>> +		offset = PMC_WAKE2_MASK;
>> +	else
>> +		offset = PMC_WAKE_MASK;
>> +
>> +	value = tegra_pmc_readl(pmc, offset);
>> +
>> +	if (on)
>> +		value |= 1 << bit;
>> +	else
>> +		value &= ~(1 << bit);
>> +
>> +	tegra_pmc_writel(pmc, value, offset);
> 
> Why the latching is done *before* writing into the WAKE registers? What
> it is latching then?

I'm looking at the TRM doc and it says that latching should be done
*after* writing to the WAKE_MASK / LEVEL registers.

Secondly it says that it's enough to do:

value = tegra_pmc_readl(pmc, PMC_CNTRL);
value |= PMC_CNTRL_LATCH_WAKEUPS;
tegra_pmc_writel(pmc, value, PMC_CNTRL);

in order to latch. There is no need for the delay and to remove the
"LATCHWAKE_EN" bit, it should be a oneshot action.

>> +	return 0;
>> +}
>> +
>>  static int tegra186_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
>>  {
>>  	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>> @@ -1954,6 +2014,49 @@ static int tegra186_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
>>  	return 0;
>>  }
>>  
>> +static int tegra210_pmc_irq_set_type(struct irq_data *data, unsigned int type)
>> +{
>> +	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>> +	unsigned int offset, bit;
>> +	u32 value;
>> +
>> +	if (data->hwirq == ULONG_MAX)
>> +		return 0;
>> +
>> +	offset = data->hwirq / 32;
>> +	bit = data->hwirq % 32;
>> +
>> +	if (data->hwirq >= 32)
>> +		offset = PMC_WAKE2_LEVEL;
>> +	else
>> +		offset = PMC_WAKE_LEVEL;
>> +
>> +	value = tegra_pmc_readl(pmc, offset);
>> +
>> +	switch (type) {
>> +	case IRQ_TYPE_EDGE_RISING:
>> +	case IRQ_TYPE_LEVEL_HIGH:
>> +		value |= 1 << bit;
>> +		break;
>> +
>> +	case IRQ_TYPE_EDGE_FALLING:
>> +	case IRQ_TYPE_LEVEL_LOW:
>> +		value &= ~(1 << bit);
>> +		break;
>> +
>> +	case IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING:
>> +		value ^= 1 << bit;
>> +		break;
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	tegra_pmc_writel(pmc, value, offset);
> 
> Shouldn't the WAKE_LEVEL be latched as well?
> 
>> +	return 0;
>> +}
>> +
>>  static int tegra186_pmc_irq_set_type(struct irq_data *data, unsigned int type)
>>  {
>>  	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>> @@ -2540,6 +2643,10 @@ static const struct pinctrl_pin_desc tegra210_pin_descs[] = {
>>  	TEGRA210_IO_PAD_TABLE(TEGRA_IO_PIN_DESC)
>>  };
>>  
>> +static const struct tegra_wake_event tegra210_wake_events[] = {
>> +	TEGRA_WAKE_IRQ("rtc", 16, 2),
>> +};
>> +
>>  static const struct tegra_pmc_soc tegra210_pmc_soc = {
>>  	.num_powergates = ARRAY_SIZE(tegra210_powergates),
>>  	.powergates = tegra210_powergates,
>> @@ -2557,10 +2664,14 @@ static const struct tegra_pmc_soc tegra210_pmc_soc = {
>>  	.regs = &tegra20_pmc_regs,
>>  	.init = tegra20_pmc_init,
>>  	.setup_irq_polarity = tegra20_pmc_setup_irq_polarity,
>> +	.irq_set_wake = tegra210_pmc_irq_set_wake,
>> +	.irq_set_type = tegra210_pmc_irq_set_type,
>>  	.reset_sources = tegra210_reset_sources,
>>  	.num_reset_sources = ARRAY_SIZE(tegra210_reset_sources),
>>  	.reset_levels = NULL,
>>  	.num_reset_levels = 0,
>> +	.num_wake_events = ARRAY_SIZE(tegra210_wake_events),
>> +	.wake_events = tegra210_wake_events,
>>  };
>>  
>>  #define TEGRA186_IO_PAD_TABLE(_pad)					     \
>>
>
Dmitry Osipenko July 23, 2019, 1:41 a.m. UTC | #3
23.07.2019 4:08, Dmitry Osipenko пишет:
> 23.07.2019 3:58, Dmitry Osipenko пишет:
>> 21.07.2019 22:40, Sowjanya Komatineni пишет:
>>> This patch implements PMC wakeup sequence for Tegra210 and defines
>>> common used RTC alarm wake event.
>>>
>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>> ---
>>>  drivers/soc/tegra/pmc.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 111 insertions(+)
>>>
>>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>>> index 91c84d0e66ae..c556f38874e1 100644
>>> --- a/drivers/soc/tegra/pmc.c
>>> +++ b/drivers/soc/tegra/pmc.c
>>> @@ -57,6 +57,12 @@
>>>  #define  PMC_CNTRL_SYSCLK_OE		BIT(11) /* system clock enable */
>>>  #define  PMC_CNTRL_SYSCLK_POLARITY	BIT(10) /* sys clk polarity */
>>>  #define  PMC_CNTRL_MAIN_RST		BIT(4)
>>> +#define  PMC_CNTRL_LATCH_WAKEUPS	BIT(5)
> 
> Please follow the TRM's bits naming.
> 
> PMC_CNTRL_LATCHWAKE_EN
> 
>>> +#define PMC_WAKE_MASK			0x0c
>>> +#define PMC_WAKE_LEVEL			0x10
>>> +#define PMC_WAKE_STATUS			0x14
>>> +#define PMC_SW_WAKE_STATUS		0x18
>>>  
>>>  #define DPD_SAMPLE			0x020
>>>  #define  DPD_SAMPLE_ENABLE		BIT(0)
>>> @@ -87,6 +93,11 @@
>>>  
>>>  #define PMC_SCRATCH41			0x140
>>>  
>>> +#define PMC_WAKE2_MASK			0x160
>>> +#define PMC_WAKE2_LEVEL			0x164
>>> +#define PMC_WAKE2_STATUS		0x168
>>> +#define PMC_SW_WAKE2_STATUS		0x16c
>>> +
>>>  #define PMC_SENSOR_CTRL			0x1b0
>>>  #define  PMC_SENSOR_CTRL_SCRATCH_WRITE	BIT(2)
>>>  #define  PMC_SENSOR_CTRL_ENABLE_RST	BIT(1)
>>> @@ -1922,6 +1933,55 @@ static const struct irq_domain_ops tegra_pmc_irq_domain_ops = {
>>>  	.alloc = tegra_pmc_irq_alloc,
>>>  };
>>>  
>>> +static int tegra210_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
>>> +{
>>> +	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>> +	unsigned int offset, bit;
>>> +	u32 value;
>>> +
>>> +	if (data->hwirq == ULONG_MAX)
>>> +		return 0;
>>> +
>>> +	offset = data->hwirq / 32;
>>> +	bit = data->hwirq % 32;
>>> +
>>> +	/*
>>> +	 * Latch wakeups to SW_WAKE_STATUS register to capture events
>>> +	 * that would not make it into wakeup event register during LP0 exit.
>>> +	 */
>>> +	value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>> +	value |= PMC_CNTRL_LATCH_WAKEUPS;
>>> +	tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>> +	udelay(120);
>>
>> Why it takes so much time to latch the values? Shouldn't some status-bit
>> be polled for the completion of latching?
>>
>> Is this register-write really getting buffered in the PMC?
>>
>>> +	value &= ~PMC_CNTRL_LATCH_WAKEUPS;
>>> +	tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>> +	udelay(120);
>>
>> 120 usecs to remove latching, really?
>>
>>> +	tegra_pmc_writel(pmc, 0, PMC_SW_WAKE_STATUS);
>>> +	tegra_pmc_writel(pmc, 0, PMC_SW_WAKE2_STATUS);
>>> +
>>> +	tegra_pmc_writel(pmc, 0, PMC_WAKE_STATUS);
>>> +	tegra_pmc_writel(pmc, 0, PMC_WAKE2_STATUS);
>>> +
>>> +	/* enable PMC wake */
>>> +	if (data->hwirq >= 32)
>>> +		offset = PMC_WAKE2_MASK;
>>> +	else
>>> +		offset = PMC_WAKE_MASK;
>>> +
>>> +	value = tegra_pmc_readl(pmc, offset);
>>> +
>>> +	if (on)
>>> +		value |= 1 << bit;
>>> +	else
>>> +		value &= ~(1 << bit);
>>> +
>>> +	tegra_pmc_writel(pmc, value, offset);
>>
>> Why the latching is done *before* writing into the WAKE registers? What
>> it is latching then?
> 
> I'm looking at the TRM doc and it says that latching should be done
> *after* writing to the WAKE_MASK / LEVEL registers.
> 
> Secondly it says that it's enough to do:
> 
> value = tegra_pmc_readl(pmc, PMC_CNTRL);
> value |= PMC_CNTRL_LATCH_WAKEUPS;
> tegra_pmc_writel(pmc, value, PMC_CNTRL);
> 
> in order to latch. There is no need for the delay and to remove the
> "LATCHWAKE_EN" bit, it should be a oneshot action.

Although, no. TRM says "stops latching on transition from 1
to 0 (sequence - set to 1,set to 0)", so it's not a oneshot action.

Have you tested this code at all? I'm wondering how it happens to work
without a proper latching.

>>> +	return 0;
>>> +}
>>> +
>>>  static int tegra186_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
>>>  {
>>>  	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>> @@ -1954,6 +2014,49 @@ static int tegra186_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
>>>  	return 0;
>>>  }
>>>  
>>> +static int tegra210_pmc_irq_set_type(struct irq_data *data, unsigned int type)
>>> +{
>>> +	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>> +	unsigned int offset, bit;
>>> +	u32 value;
>>> +
>>> +	if (data->hwirq == ULONG_MAX)
>>> +		return 0;
>>> +
>>> +	offset = data->hwirq / 32;
>>> +	bit = data->hwirq % 32;
>>> +
>>> +	if (data->hwirq >= 32)
>>> +		offset = PMC_WAKE2_LEVEL;
>>> +	else
>>> +		offset = PMC_WAKE_LEVEL;
>>> +
>>> +	value = tegra_pmc_readl(pmc, offset);
>>> +
>>> +	switch (type) {
>>> +	case IRQ_TYPE_EDGE_RISING:
>>> +	case IRQ_TYPE_LEVEL_HIGH:
>>> +		value |= 1 << bit;
>>> +		break;
>>> +
>>> +	case IRQ_TYPE_EDGE_FALLING:
>>> +	case IRQ_TYPE_LEVEL_LOW:
>>> +		value &= ~(1 << bit);
>>> +		break;
>>> +
>>> +	case IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING:
>>> +		value ^= 1 << bit;
>>> +		break;
>>> +
>>> +	default:
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	tegra_pmc_writel(pmc, value, offset);
>>
>> Shouldn't the WAKE_LEVEL be latched as well?
>>
>>> +	return 0;
>>> +}
>>> +
>>>  static int tegra186_pmc_irq_set_type(struct irq_data *data, unsigned int type)
>>>  {
>>>  	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>> @@ -2540,6 +2643,10 @@ static const struct pinctrl_pin_desc tegra210_pin_descs[] = {
>>>  	TEGRA210_IO_PAD_TABLE(TEGRA_IO_PIN_DESC)
>>>  };
>>>  
>>> +static const struct tegra_wake_event tegra210_wake_events[] = {
>>> +	TEGRA_WAKE_IRQ("rtc", 16, 2),
>>> +};
>>> +
>>>  static const struct tegra_pmc_soc tegra210_pmc_soc = {
>>>  	.num_powergates = ARRAY_SIZE(tegra210_powergates),
>>>  	.powergates = tegra210_powergates,
>>> @@ -2557,10 +2664,14 @@ static const struct tegra_pmc_soc tegra210_pmc_soc = {
>>>  	.regs = &tegra20_pmc_regs,
>>>  	.init = tegra20_pmc_init,
>>>  	.setup_irq_polarity = tegra20_pmc_setup_irq_polarity,
>>> +	.irq_set_wake = tegra210_pmc_irq_set_wake,
>>> +	.irq_set_type = tegra210_pmc_irq_set_type,
>>>  	.reset_sources = tegra210_reset_sources,
>>>  	.num_reset_sources = ARRAY_SIZE(tegra210_reset_sources),
>>>  	.reset_levels = NULL,
>>>  	.num_reset_levels = 0,
>>> +	.num_wake_events = ARRAY_SIZE(tegra210_wake_events),
>>> +	.wake_events = tegra210_wake_events,
>>>  };
>>>  
>>>  #define TEGRA186_IO_PAD_TABLE(_pad)					     \
>>>
>>
>
Dmitry Osipenko July 23, 2019, 1:52 a.m. UTC | #4
23.07.2019 4:41, Dmitry Osipenko пишет:
> 23.07.2019 4:08, Dmitry Osipenko пишет:
>> 23.07.2019 3:58, Dmitry Osipenko пишет:
>>> 21.07.2019 22:40, Sowjanya Komatineni пишет:
>>>> This patch implements PMC wakeup sequence for Tegra210 and defines
>>>> common used RTC alarm wake event.
>>>>
>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>> ---
>>>>  drivers/soc/tegra/pmc.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 111 insertions(+)
>>>>
>>>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>>>> index 91c84d0e66ae..c556f38874e1 100644
>>>> --- a/drivers/soc/tegra/pmc.c
>>>> +++ b/drivers/soc/tegra/pmc.c
>>>> @@ -57,6 +57,12 @@
>>>>  #define  PMC_CNTRL_SYSCLK_OE		BIT(11) /* system clock enable */
>>>>  #define  PMC_CNTRL_SYSCLK_POLARITY	BIT(10) /* sys clk polarity */
>>>>  #define  PMC_CNTRL_MAIN_RST		BIT(4)
>>>> +#define  PMC_CNTRL_LATCH_WAKEUPS	BIT(5)
>>
>> Please follow the TRM's bits naming.
>>
>> PMC_CNTRL_LATCHWAKE_EN
>>
>>>> +#define PMC_WAKE_MASK			0x0c
>>>> +#define PMC_WAKE_LEVEL			0x10
>>>> +#define PMC_WAKE_STATUS			0x14
>>>> +#define PMC_SW_WAKE_STATUS		0x18
>>>>  
>>>>  #define DPD_SAMPLE			0x020
>>>>  #define  DPD_SAMPLE_ENABLE		BIT(0)
>>>> @@ -87,6 +93,11 @@
>>>>  
>>>>  #define PMC_SCRATCH41			0x140
>>>>  
>>>> +#define PMC_WAKE2_MASK			0x160
>>>> +#define PMC_WAKE2_LEVEL			0x164
>>>> +#define PMC_WAKE2_STATUS		0x168
>>>> +#define PMC_SW_WAKE2_STATUS		0x16c
>>>> +
>>>>  #define PMC_SENSOR_CTRL			0x1b0
>>>>  #define  PMC_SENSOR_CTRL_SCRATCH_WRITE	BIT(2)
>>>>  #define  PMC_SENSOR_CTRL_ENABLE_RST	BIT(1)
>>>> @@ -1922,6 +1933,55 @@ static const struct irq_domain_ops tegra_pmc_irq_domain_ops = {
>>>>  	.alloc = tegra_pmc_irq_alloc,
>>>>  };
>>>>  
>>>> +static int tegra210_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
>>>> +{
>>>> +	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>>> +	unsigned int offset, bit;
>>>> +	u32 value;
>>>> +
>>>> +	if (data->hwirq == ULONG_MAX)
>>>> +		return 0;
>>>> +
>>>> +	offset = data->hwirq / 32;
>>>> +	bit = data->hwirq % 32;
>>>> +
>>>> +	/*
>>>> +	 * Latch wakeups to SW_WAKE_STATUS register to capture events
>>>> +	 * that would not make it into wakeup event register during LP0 exit.
>>>> +	 */
>>>> +	value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>>> +	value |= PMC_CNTRL_LATCH_WAKEUPS;
>>>> +	tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>> +	udelay(120);
>>>
>>> Why it takes so much time to latch the values? Shouldn't some status-bit
>>> be polled for the completion of latching?
>>>
>>> Is this register-write really getting buffered in the PMC?
>>>
>>>> +	value &= ~PMC_CNTRL_LATCH_WAKEUPS;
>>>> +	tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>> +	udelay(120);
>>>
>>> 120 usecs to remove latching, really?
>>>
>>>> +	tegra_pmc_writel(pmc, 0, PMC_SW_WAKE_STATUS);
>>>> +	tegra_pmc_writel(pmc, 0, PMC_SW_WAKE2_STATUS);
>>>> +
>>>> +	tegra_pmc_writel(pmc, 0, PMC_WAKE_STATUS);
>>>> +	tegra_pmc_writel(pmc, 0, PMC_WAKE2_STATUS);
>>>> +
>>>> +	/* enable PMC wake */
>>>> +	if (data->hwirq >= 32)
>>>> +		offset = PMC_WAKE2_MASK;
>>>> +	else
>>>> +		offset = PMC_WAKE_MASK;
>>>> +
>>>> +	value = tegra_pmc_readl(pmc, offset);
>>>> +
>>>> +	if (on)
>>>> +		value |= 1 << bit;
>>>> +	else
>>>> +		value &= ~(1 << bit);
>>>> +
>>>> +	tegra_pmc_writel(pmc, value, offset);
>>>
>>> Why the latching is done *before* writing into the WAKE registers? What
>>> it is latching then?
>>
>> I'm looking at the TRM doc and it says that latching should be done
>> *after* writing to the WAKE_MASK / LEVEL registers.
>>
>> Secondly it says that it's enough to do:
>>
>> value = tegra_pmc_readl(pmc, PMC_CNTRL);
>> value |= PMC_CNTRL_LATCH_WAKEUPS;
>> tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>
>> in order to latch. There is no need for the delay and to remove the
>> "LATCHWAKE_EN" bit, it should be a oneshot action.
> 
> Although, no. TRM says "stops latching on transition from 1
> to 0 (sequence - set to 1,set to 0)", so it's not a oneshot action.
> 
> Have you tested this code at all? I'm wondering how it happens to work
> without a proper latching.

Okay, I re-read the TRM and apparently "latching" just means storing of
WAKE-event bit in the WAKE-status register if latching is enabled. Hence
the PMC_CNTRL_LATCHWAKE_EN should be enabled in tegra_pmc_suspend() and
unset in tegra_pmc_resume().

Also, apparently, on resume from suspend the interrupt should be
re-triggered in accordance to the WAKE-status, then the WAKE-status need
to be cleared.

>>>> +	return 0;
>>>> +}
>>>> +
>>>>  static int tegra186_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
>>>>  {
>>>>  	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>>> @@ -1954,6 +2014,49 @@ static int tegra186_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +static int tegra210_pmc_irq_set_type(struct irq_data *data, unsigned int type)
>>>> +{
>>>> +	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>>> +	unsigned int offset, bit;
>>>> +	u32 value;
>>>> +
>>>> +	if (data->hwirq == ULONG_MAX)
>>>> +		return 0;
>>>> +
>>>> +	offset = data->hwirq / 32;
>>>> +	bit = data->hwirq % 32;
>>>> +
>>>> +	if (data->hwirq >= 32)
>>>> +		offset = PMC_WAKE2_LEVEL;
>>>> +	else
>>>> +		offset = PMC_WAKE_LEVEL;
>>>> +
>>>> +	value = tegra_pmc_readl(pmc, offset);
>>>> +
>>>> +	switch (type) {
>>>> +	case IRQ_TYPE_EDGE_RISING:
>>>> +	case IRQ_TYPE_LEVEL_HIGH:
>>>> +		value |= 1 << bit;
>>>> +		break;
>>>> +
>>>> +	case IRQ_TYPE_EDGE_FALLING:
>>>> +	case IRQ_TYPE_LEVEL_LOW:
>>>> +		value &= ~(1 << bit);
>>>> +		break;
>>>> +
>>>> +	case IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING:
>>>> +		value ^= 1 << bit;
>>>> +		break;
>>>> +
>>>> +	default:
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	tegra_pmc_writel(pmc, value, offset);
>>>
>>> Shouldn't the WAKE_LEVEL be latched as well?
>>>
>>>> +	return 0;
>>>> +}
>>>> +
>>>>  static int tegra186_pmc_irq_set_type(struct irq_data *data, unsigned int type)
>>>>  {
>>>>  	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>>> @@ -2540,6 +2643,10 @@ static const struct pinctrl_pin_desc tegra210_pin_descs[] = {
>>>>  	TEGRA210_IO_PAD_TABLE(TEGRA_IO_PIN_DESC)
>>>>  };
>>>>  
>>>> +static const struct tegra_wake_event tegra210_wake_events[] = {
>>>> +	TEGRA_WAKE_IRQ("rtc", 16, 2),
>>>> +};
>>>> +
>>>>  static const struct tegra_pmc_soc tegra210_pmc_soc = {
>>>>  	.num_powergates = ARRAY_SIZE(tegra210_powergates),
>>>>  	.powergates = tegra210_powergates,
>>>> @@ -2557,10 +2664,14 @@ static const struct tegra_pmc_soc tegra210_pmc_soc = {
>>>>  	.regs = &tegra20_pmc_regs,
>>>>  	.init = tegra20_pmc_init,
>>>>  	.setup_irq_polarity = tegra20_pmc_setup_irq_polarity,
>>>> +	.irq_set_wake = tegra210_pmc_irq_set_wake,
>>>> +	.irq_set_type = tegra210_pmc_irq_set_type,
>>>>  	.reset_sources = tegra210_reset_sources,
>>>>  	.num_reset_sources = ARRAY_SIZE(tegra210_reset_sources),
>>>>  	.reset_levels = NULL,
>>>>  	.num_reset_levels = 0,
>>>> +	.num_wake_events = ARRAY_SIZE(tegra210_wake_events),
>>>> +	.wake_events = tegra210_wake_events,
>>>>  };
>>>>  
>>>>  #define TEGRA186_IO_PAD_TABLE(_pad)					     \
>>>>
>>>
>>
>
Dmitry Osipenko July 23, 2019, 2:10 a.m. UTC | #5
23.07.2019 4:52, Dmitry Osipenko пишет:
> 23.07.2019 4:41, Dmitry Osipenko пишет:
>> 23.07.2019 4:08, Dmitry Osipenko пишет:
>>> 23.07.2019 3:58, Dmitry Osipenko пишет:
>>>> 21.07.2019 22:40, Sowjanya Komatineni пишет:
>>>>> This patch implements PMC wakeup sequence for Tegra210 and defines
>>>>> common used RTC alarm wake event.
>>>>>
>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>> ---
>>>>>  drivers/soc/tegra/pmc.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 111 insertions(+)
>>>>>
>>>>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>>>>> index 91c84d0e66ae..c556f38874e1 100644
>>>>> --- a/drivers/soc/tegra/pmc.c
>>>>> +++ b/drivers/soc/tegra/pmc.c
>>>>> @@ -57,6 +57,12 @@
>>>>>  #define  PMC_CNTRL_SYSCLK_OE		BIT(11) /* system clock enable */
>>>>>  #define  PMC_CNTRL_SYSCLK_POLARITY	BIT(10) /* sys clk polarity */
>>>>>  #define  PMC_CNTRL_MAIN_RST		BIT(4)
>>>>> +#define  PMC_CNTRL_LATCH_WAKEUPS	BIT(5)
>>>
>>> Please follow the TRM's bits naming.
>>>
>>> PMC_CNTRL_LATCHWAKE_EN
>>>
>>>>> +#define PMC_WAKE_MASK			0x0c
>>>>> +#define PMC_WAKE_LEVEL			0x10
>>>>> +#define PMC_WAKE_STATUS			0x14
>>>>> +#define PMC_SW_WAKE_STATUS		0x18
>>>>>  
>>>>>  #define DPD_SAMPLE			0x020
>>>>>  #define  DPD_SAMPLE_ENABLE		BIT(0)
>>>>> @@ -87,6 +93,11 @@
>>>>>  
>>>>>  #define PMC_SCRATCH41			0x140
>>>>>  
>>>>> +#define PMC_WAKE2_MASK			0x160
>>>>> +#define PMC_WAKE2_LEVEL			0x164
>>>>> +#define PMC_WAKE2_STATUS		0x168
>>>>> +#define PMC_SW_WAKE2_STATUS		0x16c
>>>>> +
>>>>>  #define PMC_SENSOR_CTRL			0x1b0
>>>>>  #define  PMC_SENSOR_CTRL_SCRATCH_WRITE	BIT(2)
>>>>>  #define  PMC_SENSOR_CTRL_ENABLE_RST	BIT(1)
>>>>> @@ -1922,6 +1933,55 @@ static const struct irq_domain_ops tegra_pmc_irq_domain_ops = {
>>>>>  	.alloc = tegra_pmc_irq_alloc,
>>>>>  };
>>>>>  
>>>>> +static int tegra210_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
>>>>> +{
>>>>> +	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>>>> +	unsigned int offset, bit;
>>>>> +	u32 value;
>>>>> +
>>>>> +	if (data->hwirq == ULONG_MAX)
>>>>> +		return 0;
>>>>> +
>>>>> +	offset = data->hwirq / 32;
>>>>> +	bit = data->hwirq % 32;
>>>>> +
>>>>> +	/*
>>>>> +	 * Latch wakeups to SW_WAKE_STATUS register to capture events
>>>>> +	 * that would not make it into wakeup event register during LP0 exit.
>>>>> +	 */
>>>>> +	value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>>>> +	value |= PMC_CNTRL_LATCH_WAKEUPS;
>>>>> +	tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>>> +	udelay(120);
>>>>
>>>> Why it takes so much time to latch the values? Shouldn't some status-bit
>>>> be polled for the completion of latching?
>>>>
>>>> Is this register-write really getting buffered in the PMC?
>>>>
>>>>> +	value &= ~PMC_CNTRL_LATCH_WAKEUPS;
>>>>> +	tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>>> +	udelay(120);
>>>>
>>>> 120 usecs to remove latching, really?
>>>>
>>>>> +	tegra_pmc_writel(pmc, 0, PMC_SW_WAKE_STATUS);
>>>>> +	tegra_pmc_writel(pmc, 0, PMC_SW_WAKE2_STATUS);
>>>>> +
>>>>> +	tegra_pmc_writel(pmc, 0, PMC_WAKE_STATUS);
>>>>> +	tegra_pmc_writel(pmc, 0, PMC_WAKE2_STATUS);
>>>>> +
>>>>> +	/* enable PMC wake */
>>>>> +	if (data->hwirq >= 32)
>>>>> +		offset = PMC_WAKE2_MASK;
>>>>> +	else
>>>>> +		offset = PMC_WAKE_MASK;
>>>>> +
>>>>> +	value = tegra_pmc_readl(pmc, offset);
>>>>> +
>>>>> +	if (on)
>>>>> +		value |= 1 << bit;
>>>>> +	else
>>>>> +		value &= ~(1 << bit);
>>>>> +
>>>>> +	tegra_pmc_writel(pmc, value, offset);
>>>>
>>>> Why the latching is done *before* writing into the WAKE registers? What
>>>> it is latching then?
>>>
>>> I'm looking at the TRM doc and it says that latching should be done
>>> *after* writing to the WAKE_MASK / LEVEL registers.
>>>
>>> Secondly it says that it's enough to do:
>>>
>>> value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>> value |= PMC_CNTRL_LATCH_WAKEUPS;
>>> tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>
>>> in order to latch. There is no need for the delay and to remove the
>>> "LATCHWAKE_EN" bit, it should be a oneshot action.
>>
>> Although, no. TRM says "stops latching on transition from 1
>> to 0 (sequence - set to 1,set to 0)", so it's not a oneshot action.
>>
>> Have you tested this code at all? I'm wondering how it happens to work
>> without a proper latching.
> 
> Okay, I re-read the TRM and apparently "latching" just means storing of
> WAKE-event bit in the WAKE-status register if latching is enabled. Hence
> the PMC_CNTRL_LATCHWAKE_EN should be enabled in tegra_pmc_suspend() and
> unset in tegra_pmc_resume().
> 
> Also, apparently, on resume from suspend the interrupt should be
> re-triggered in accordance to the WAKE-status, then the WAKE-status need
> to be cleared.

I'm now also recalling that downstream kernel had some problems in
regards to missing power-button presses on resume from suspend because
input driver reads the GPIO-key state in order to determine the press
status and the GPIO state in already unset at the time when input driver
resumes. Hence it happened sometime that after pressing power button,
device waked up from LP0 and then immediately went into suspend (due to
android's wakelocks).

>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>>  static int tegra186_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
>>>>>  {
>>>>>  	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>>>> @@ -1954,6 +2014,49 @@ static int tegra186_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
>>>>>  	return 0;
>>>>>  }
>>>>>  
>>>>> +static int tegra210_pmc_irq_set_type(struct irq_data *data, unsigned int type)
>>>>> +{
>>>>> +	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>>>> +	unsigned int offset, bit;
>>>>> +	u32 value;
>>>>> +
>>>>> +	if (data->hwirq == ULONG_MAX)
>>>>> +		return 0;
>>>>> +
>>>>> +	offset = data->hwirq / 32;
>>>>> +	bit = data->hwirq % 32;
>>>>> +
>>>>> +	if (data->hwirq >= 32)
>>>>> +		offset = PMC_WAKE2_LEVEL;
>>>>> +	else
>>>>> +		offset = PMC_WAKE_LEVEL;
>>>>> +
>>>>> +	value = tegra_pmc_readl(pmc, offset);
>>>>> +
>>>>> +	switch (type) {
>>>>> +	case IRQ_TYPE_EDGE_RISING:
>>>>> +	case IRQ_TYPE_LEVEL_HIGH:
>>>>> +		value |= 1 << bit;
>>>>> +		break;
>>>>> +
>>>>> +	case IRQ_TYPE_EDGE_FALLING:
>>>>> +	case IRQ_TYPE_LEVEL_LOW:
>>>>> +		value &= ~(1 << bit);
>>>>> +		break;
>>>>> +
>>>>> +	case IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING:
>>>>> +		value ^= 1 << bit;
>>>>> +		break;
>>>>> +
>>>>> +	default:
>>>>> +		return -EINVAL;
>>>>> +	}
>>>>> +
>>>>> +	tegra_pmc_writel(pmc, value, offset);
>>>>
>>>> Shouldn't the WAKE_LEVEL be latched as well?
>>>>
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>>  static int tegra186_pmc_irq_set_type(struct irq_data *data, unsigned int type)
>>>>>  {
>>>>>  	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>>>> @@ -2540,6 +2643,10 @@ static const struct pinctrl_pin_desc tegra210_pin_descs[] = {
>>>>>  	TEGRA210_IO_PAD_TABLE(TEGRA_IO_PIN_DESC)
>>>>>  };
>>>>>  
>>>>> +static const struct tegra_wake_event tegra210_wake_events[] = {
>>>>> +	TEGRA_WAKE_IRQ("rtc", 16, 2),
>>>>> +};
>>>>> +
>>>>>  static const struct tegra_pmc_soc tegra210_pmc_soc = {
>>>>>  	.num_powergates = ARRAY_SIZE(tegra210_powergates),
>>>>>  	.powergates = tegra210_powergates,
>>>>> @@ -2557,10 +2664,14 @@ static const struct tegra_pmc_soc tegra210_pmc_soc = {
>>>>>  	.regs = &tegra20_pmc_regs,
>>>>>  	.init = tegra20_pmc_init,
>>>>>  	.setup_irq_polarity = tegra20_pmc_setup_irq_polarity,
>>>>> +	.irq_set_wake = tegra210_pmc_irq_set_wake,
>>>>> +	.irq_set_type = tegra210_pmc_irq_set_type,
>>>>>  	.reset_sources = tegra210_reset_sources,
>>>>>  	.num_reset_sources = ARRAY_SIZE(tegra210_reset_sources),
>>>>>  	.reset_levels = NULL,
>>>>>  	.num_reset_levels = 0,
>>>>> +	.num_wake_events = ARRAY_SIZE(tegra210_wake_events),
>>>>> +	.wake_events = tegra210_wake_events,
>>>>>  };
>>>>>  
>>>>>  #define TEGRA186_IO_PAD_TABLE(_pad)					     \
>>>>>
>>>>
>>>
>>
>
Dmitry Osipenko July 23, 2019, 3:03 a.m. UTC | #6
23.07.2019 4:52, Sowjanya Komatineni пишет:
> 
> On 7/22/19 6:41 PM, Dmitry Osipenko wrote:
>> 23.07.2019 4:08, Dmitry Osipenko пишет:
>>> 23.07.2019 3:58, Dmitry Osipenko пишет:
>>>> 21.07.2019 22:40, Sowjanya Komatineni пишет:
>>>>> This patch implements PMC wakeup sequence for Tegra210 and defines
>>>>> common used RTC alarm wake event.
>>>>>
>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>> ---
>>>>>  drivers/soc/tegra/pmc.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 111 insertions(+)
>>>>>
>>>>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>>>>> index 91c84d0e66ae..c556f38874e1 100644
>>>>> --- a/drivers/soc/tegra/pmc.c
>>>>> +++ b/drivers/soc/tegra/pmc.c
>>>>> @@ -57,6 +57,12 @@
>>>>>  #define  PMC_CNTRL_SYSCLK_OE		BIT(11) /* system clock enable */
>>>>>  #define  PMC_CNTRL_SYSCLK_POLARITY	BIT(10) /* sys clk polarity */
>>>>>  #define  PMC_CNTRL_MAIN_RST		BIT(4)
>>>>> +#define  PMC_CNTRL_LATCH_WAKEUPS	BIT(5)
>>> Please follow the TRM's bits naming.
>>>
>>> PMC_CNTRL_LATCHWAKE_EN
>>>
>>>>> +#define PMC_WAKE_MASK			0x0c
>>>>> +#define PMC_WAKE_LEVEL			0x10
>>>>> +#define PMC_WAKE_STATUS			0x14
>>>>> +#define PMC_SW_WAKE_STATUS		0x18
>>>>>  
>>>>>  #define DPD_SAMPLE			0x020
>>>>>  #define  DPD_SAMPLE_ENABLE		BIT(0)
>>>>> @@ -87,6 +93,11 @@
>>>>>  
>>>>>  #define PMC_SCRATCH41			0x140
>>>>>  
>>>>> +#define PMC_WAKE2_MASK			0x160
>>>>> +#define PMC_WAKE2_LEVEL			0x164
>>>>> +#define PMC_WAKE2_STATUS		0x168
>>>>> +#define PMC_SW_WAKE2_STATUS		0x16c
>>>>> +
>>>>>  #define PMC_SENSOR_CTRL			0x1b0
>>>>>  #define  PMC_SENSOR_CTRL_SCRATCH_WRITE	BIT(2)
>>>>>  #define  PMC_SENSOR_CTRL_ENABLE_RST	BIT(1)
>>>>> @@ -1922,6 +1933,55 @@ static const struct irq_domain_ops tegra_pmc_irq_domain_ops = {
>>>>>  	.alloc = tegra_pmc_irq_alloc,
>>>>>  };
>>>>>  
>>>>> +static int tegra210_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
>>>>> +{
>>>>> +	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>>>> +	unsigned int offset, bit;
>>>>> +	u32 value;
>>>>> +
>>>>> +	if (data->hwirq == ULONG_MAX)
>>>>> +		return 0;
>>>>> +
>>>>> +	offset = data->hwirq / 32;
>>>>> +	bit = data->hwirq % 32;
>>>>> +
>>>>> +	/*
>>>>> +	 * Latch wakeups to SW_WAKE_STATUS register to capture events
>>>>> +	 * that would not make it into wakeup event register during LP0 exit.
>>>>> +	 */
>>>>> +	value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>>>> +	value |= PMC_CNTRL_LATCH_WAKEUPS;
>>>>> +	tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>>> +	udelay(120);
>>>> Why it takes so much time to latch the values? Shouldn't some status-bit
>>>> be polled for the completion of latching?
>>>>
>>>> Is this register-write really getting buffered in the PMC?
>>>>
>>>>> +	value &= ~PMC_CNTRL_LATCH_WAKEUPS;
>>>>> +	tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>>> +	udelay(120);
>>>> 120 usecs to remove latching, really?
>>>>
>>>>> +	tegra_pmc_writel(pmc, 0, PMC_SW_WAKE_STATUS);
>>>>> +	tegra_pmc_writel(pmc, 0, PMC_SW_WAKE2_STATUS);
>>>>> +
>>>>> +	tegra_pmc_writel(pmc, 0, PMC_WAKE_STATUS);
>>>>> +	tegra_pmc_writel(pmc, 0, PMC_WAKE2_STATUS);
>>>>> +
>>>>> +	/* enable PMC wake */
>>>>> +	if (data->hwirq >= 32)
>>>>> +		offset = PMC_WAKE2_MASK;
>>>>> +	else
>>>>> +		offset = PMC_WAKE_MASK;
>>>>> +
>>>>> +	value = tegra_pmc_readl(pmc, offset);
>>>>> +
>>>>> +	if (on)
>>>>> +		value |= 1 << bit;
>>>>> +	else
>>>>> +		value &= ~(1 << bit);
>>>>> +
>>>>> +	tegra_pmc_writel(pmc, value, offset);
>>>> Why the latching is done *before* writing into the WAKE registers? What
>>>> it is latching then?
>>> I'm looking at the TRM doc and it says that latching should be done
>>> *after* writing to the WAKE_MASK / LEVEL registers.
>>>
>>> Secondly it says that it's enough to do:
>>>
>>> value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>> value |= PMC_CNTRL_LATCH_WAKEUPS;
>>> tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>
>>> in order to latch. There is no need for the delay and to remove the
>>> "LATCHWAKE_EN" bit, it should be a oneshot action.
>> Although, no. TRM says "stops latching on transition from 1
>> to 0 (sequence - set to 1,set to 0)", so it's not a oneshot action.
>>
>> Have you tested this code at all? I'm wondering how it happens to work
>> without a proper latching.
> Yes, ofcourse its tested and this sequence to do transition is
> recommendation from Tegra designer.
> Will check if TRM doesn't have update properly or will re-confirm
> internally on delay time...
> 
> On any of the wake event PMC wakeup happens and WAKE_STATUS register
> will have bits set for all events that triggered wake.
> After wakeup PMC doesn't update SW_WAKE_STATUS register as per PMC design.
> SW latch register added in design helps to provide a way to capture
> those events that happen right during wakeup time and didnt make it to
> SW_WAKE_STATUS register.
> So before next suspend entry, latching all prior wake events into SW
> WAKE_STATUS and then clearing them.

I'm now wondering whether the latching cold be turned ON permanently
during of the PMC's probe, for simplicity.

> LATCHWAKE_EN - When set, enables latching and stops latching on
> transition from 1 to 0
> There is recommendation of min 120uSec for this transition to stop
> latching. Will double-check why 120uSec

Yes, please check.

>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>>  static int tegra186_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
>>>>>  {
>>>>>  	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>>>> @@ -1954,6 +2014,49 @@ static int tegra186_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
>>>>>  	return 0;
>>>>>  }
>>>>>  
>>>>> +static int tegra210_pmc_irq_set_type(struct irq_data *data, unsigned int type)
>>>>> +{
>>>>> +	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>>>> +	unsigned int offset, bit;
>>>>> +	u32 value;
>>>>> +
>>>>> +	if (data->hwirq == ULONG_MAX)
>>>>> +		return 0;
>>>>> +
>>>>> +	offset = data->hwirq / 32;
>>>>> +	bit = data->hwirq % 32;
>>>>> +
>>>>> +	if (data->hwirq >= 32)
>>>>> +		offset = PMC_WAKE2_LEVEL;
>>>>> +	else
>>>>> +		offset = PMC_WAKE_LEVEL;
>>>>> +
>>>>> +	value = tegra_pmc_readl(pmc, offset);
>>>>> +
>>>>> +	switch (type) {
>>>>> +	case IRQ_TYPE_EDGE_RISING:
>>>>> +	case IRQ_TYPE_LEVEL_HIGH:
>>>>> +		value |= 1 << bit;
>>>>> +		break;
>>>>> +
>>>>> +	case IRQ_TYPE_EDGE_FALLING:
>>>>> +	case IRQ_TYPE_LEVEL_LOW:
>>>>> +		value &= ~(1 << bit);
>>>>> +		break;
>>>>> +
>>>>> +	case IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING:
>>>>> +		value ^= 1 << bit;
>>>>> +		break;
>>>>> +
>>>>> +	default:
>>>>> +		return -EINVAL;
>>>>> +	}
>>>>> +
>>>>> +	tegra_pmc_writel(pmc, value, offset);
>>>> Shouldn't the WAKE_LEVEL be latched as well?
> WAKE_LEVELs dont need any latch as they are the levels SW sets for wake
> trigger and they are not status

Okay.

[snip]
Sowjanya Komatineni July 23, 2019, 3:09 a.m. UTC | #7
On 7/22/19 8:03 PM, Dmitry Osipenko wrote:
> 23.07.2019 4:52, Sowjanya Komatineni пишет:
>> On 7/22/19 6:41 PM, Dmitry Osipenko wrote:
>>> 23.07.2019 4:08, Dmitry Osipenko пишет:
>>>> 23.07.2019 3:58, Dmitry Osipenko пишет:
>>>>> 21.07.2019 22:40, Sowjanya Komatineni пишет:
>>>>>> This patch implements PMC wakeup sequence for Tegra210 and defines
>>>>>> common used RTC alarm wake event.
>>>>>>
>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>> ---
>>>>>>   drivers/soc/tegra/pmc.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>   1 file changed, 111 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>>>>>> index 91c84d0e66ae..c556f38874e1 100644
>>>>>> --- a/drivers/soc/tegra/pmc.c
>>>>>> +++ b/drivers/soc/tegra/pmc.c
>>>>>> @@ -57,6 +57,12 @@
>>>>>>   #define  PMC_CNTRL_SYSCLK_OE		BIT(11) /* system clock enable */
>>>>>>   #define  PMC_CNTRL_SYSCLK_POLARITY	BIT(10) /* sys clk polarity */
>>>>>>   #define  PMC_CNTRL_MAIN_RST		BIT(4)
>>>>>> +#define  PMC_CNTRL_LATCH_WAKEUPS	BIT(5)
>>>> Please follow the TRM's bits naming.
>>>>
>>>> PMC_CNTRL_LATCHWAKE_EN
>>>>
>>>>>> +#define PMC_WAKE_MASK			0x0c
>>>>>> +#define PMC_WAKE_LEVEL			0x10
>>>>>> +#define PMC_WAKE_STATUS			0x14
>>>>>> +#define PMC_SW_WAKE_STATUS		0x18
>>>>>>   
>>>>>>   #define DPD_SAMPLE			0x020
>>>>>>   #define  DPD_SAMPLE_ENABLE		BIT(0)
>>>>>> @@ -87,6 +93,11 @@
>>>>>>   
>>>>>>   #define PMC_SCRATCH41			0x140
>>>>>>   
>>>>>> +#define PMC_WAKE2_MASK			0x160
>>>>>> +#define PMC_WAKE2_LEVEL			0x164
>>>>>> +#define PMC_WAKE2_STATUS		0x168
>>>>>> +#define PMC_SW_WAKE2_STATUS		0x16c
>>>>>> +
>>>>>>   #define PMC_SENSOR_CTRL			0x1b0
>>>>>>   #define  PMC_SENSOR_CTRL_SCRATCH_WRITE	BIT(2)
>>>>>>   #define  PMC_SENSOR_CTRL_ENABLE_RST	BIT(1)
>>>>>> @@ -1922,6 +1933,55 @@ static const struct irq_domain_ops tegra_pmc_irq_domain_ops = {
>>>>>>   	.alloc = tegra_pmc_irq_alloc,
>>>>>>   };
>>>>>>   
>>>>>> +static int tegra210_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
>>>>>> +{
>>>>>> +	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>>>>> +	unsigned int offset, bit;
>>>>>> +	u32 value;
>>>>>> +
>>>>>> +	if (data->hwirq == ULONG_MAX)
>>>>>> +		return 0;
>>>>>> +
>>>>>> +	offset = data->hwirq / 32;
>>>>>> +	bit = data->hwirq % 32;
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * Latch wakeups to SW_WAKE_STATUS register to capture events
>>>>>> +	 * that would not make it into wakeup event register during LP0 exit.
>>>>>> +	 */
>>>>>> +	value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>>>>> +	value |= PMC_CNTRL_LATCH_WAKEUPS;
>>>>>> +	tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>>>> +	udelay(120);
>>>>> Why it takes so much time to latch the values? Shouldn't some status-bit
>>>>> be polled for the completion of latching?
>>>>>
>>>>> Is this register-write really getting buffered in the PMC?
>>>>>
>>>>>> +	value &= ~PMC_CNTRL_LATCH_WAKEUPS;
>>>>>> +	tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>>>> +	udelay(120);
>>>>> 120 usecs to remove latching, really?
>>>>>
>>>>>> +	tegra_pmc_writel(pmc, 0, PMC_SW_WAKE_STATUS);
>>>>>> +	tegra_pmc_writel(pmc, 0, PMC_SW_WAKE2_STATUS);
>>>>>> +
>>>>>> +	tegra_pmc_writel(pmc, 0, PMC_WAKE_STATUS);
>>>>>> +	tegra_pmc_writel(pmc, 0, PMC_WAKE2_STATUS);
>>>>>> +
>>>>>> +	/* enable PMC wake */
>>>>>> +	if (data->hwirq >= 32)
>>>>>> +		offset = PMC_WAKE2_MASK;
>>>>>> +	else
>>>>>> +		offset = PMC_WAKE_MASK;
>>>>>> +
>>>>>> +	value = tegra_pmc_readl(pmc, offset);
>>>>>> +
>>>>>> +	if (on)
>>>>>> +		value |= 1 << bit;
>>>>>> +	else
>>>>>> +		value &= ~(1 << bit);
>>>>>> +
>>>>>> +	tegra_pmc_writel(pmc, value, offset);
>>>>> Why the latching is done *before* writing into the WAKE registers? What
>>>>> it is latching then?
>>>> I'm looking at the TRM doc and it says that latching should be done
>>>> *after* writing to the WAKE_MASK / LEVEL registers.
>>>>
>>>> Secondly it says that it's enough to do:
>>>>
>>>> value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>>> value |= PMC_CNTRL_LATCH_WAKEUPS;
>>>> tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>>
>>>> in order to latch. There is no need for the delay and to remove the
>>>> "LATCHWAKE_EN" bit, it should be a oneshot action.
>>> Although, no. TRM says "stops latching on transition from 1
>>> to 0 (sequence - set to 1,set to 0)", so it's not a oneshot action.
>>>
>>> Have you tested this code at all? I'm wondering how it happens to work
>>> without a proper latching.
>> Yes, ofcourse its tested and this sequence to do transition is
>> recommendation from Tegra designer.
>> Will check if TRM doesn't have update properly or will re-confirm
>> internally on delay time...
>>
>> On any of the wake event PMC wakeup happens and WAKE_STATUS register
>> will have bits set for all events that triggered wake.
>> After wakeup PMC doesn't update SW_WAKE_STATUS register as per PMC design.
>> SW latch register added in design helps to provide a way to capture
>> those events that happen right during wakeup time and didnt make it to
>> SW_WAKE_STATUS register.
>> So before next suspend entry, latching all prior wake events into SW
>> WAKE_STATUS and then clearing them.
> I'm now wondering whether the latching cold be turned ON permanently
> during of the PMC's probe, for simplicity.
latching should be done on suspend-resume cycle as wake events gets 
generates on every suspend-resume cycle.
>> LATCHWAKE_EN - When set, enables latching and stops latching on
>> transition from 1 to 0
>> There is recommendation of min 120uSec for this transition to stop
>> latching. Will double-check why 120uSec
> Yes, please check.
>
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>>   static int tegra186_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
>>>>>>   {
>>>>>>   	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>>>>> @@ -1954,6 +2014,49 @@ static int tegra186_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
>>>>>>   	return 0;
>>>>>>   }
>>>>>>   
>>>>>> +static int tegra210_pmc_irq_set_type(struct irq_data *data, unsigned int type)
>>>>>> +{
>>>>>> +	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>>>>> +	unsigned int offset, bit;
>>>>>> +	u32 value;
>>>>>> +
>>>>>> +	if (data->hwirq == ULONG_MAX)
>>>>>> +		return 0;
>>>>>> +
>>>>>> +	offset = data->hwirq / 32;
>>>>>> +	bit = data->hwirq % 32;
>>>>>> +
>>>>>> +	if (data->hwirq >= 32)
>>>>>> +		offset = PMC_WAKE2_LEVEL;
>>>>>> +	else
>>>>>> +		offset = PMC_WAKE_LEVEL;
>>>>>> +
>>>>>> +	value = tegra_pmc_readl(pmc, offset);
>>>>>> +
>>>>>> +	switch (type) {
>>>>>> +	case IRQ_TYPE_EDGE_RISING:
>>>>>> +	case IRQ_TYPE_LEVEL_HIGH:
>>>>>> +		value |= 1 << bit;
>>>>>> +		break;
>>>>>> +
>>>>>> +	case IRQ_TYPE_EDGE_FALLING:
>>>>>> +	case IRQ_TYPE_LEVEL_LOW:
>>>>>> +		value &= ~(1 << bit);
>>>>>> +		break;
>>>>>> +
>>>>>> +	case IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING:
>>>>>> +		value ^= 1 << bit;
>>>>>> +		break;
>>>>>> +
>>>>>> +	default:
>>>>>> +		return -EINVAL;
>>>>>> +	}
>>>>>> +
>>>>>> +	tegra_pmc_writel(pmc, value, offset);
>>>>> Shouldn't the WAKE_LEVEL be latched as well?
>> WAKE_LEVELs dont need any latch as they are the levels SW sets for wake
>> trigger and they are not status
> Okay.
>
> [snip]
Dmitry Osipenko July 23, 2019, 3:25 a.m. UTC | #8
23.07.2019 6:09, Sowjanya Komatineni пишет:
> 
> On 7/22/19 8:03 PM, Dmitry Osipenko wrote:
>> 23.07.2019 4:52, Sowjanya Komatineni пишет:
>>> On 7/22/19 6:41 PM, Dmitry Osipenko wrote:
>>>> 23.07.2019 4:08, Dmitry Osipenko пишет:
>>>>> 23.07.2019 3:58, Dmitry Osipenko пишет:
>>>>>> 21.07.2019 22:40, Sowjanya Komatineni пишет:
>>>>>>> This patch implements PMC wakeup sequence for Tegra210 and defines
>>>>>>> common used RTC alarm wake event.
>>>>>>>
>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>> ---
>>>>>>>   drivers/soc/tegra/pmc.c | 111
>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>   1 file changed, 111 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>>>>>>> index 91c84d0e66ae..c556f38874e1 100644
>>>>>>> --- a/drivers/soc/tegra/pmc.c
>>>>>>> +++ b/drivers/soc/tegra/pmc.c
>>>>>>> @@ -57,6 +57,12 @@
>>>>>>>   #define  PMC_CNTRL_SYSCLK_OE        BIT(11) /* system clock
>>>>>>> enable */
>>>>>>>   #define  PMC_CNTRL_SYSCLK_POLARITY    BIT(10) /* sys clk
>>>>>>> polarity */
>>>>>>>   #define  PMC_CNTRL_MAIN_RST        BIT(4)
>>>>>>> +#define  PMC_CNTRL_LATCH_WAKEUPS    BIT(5)
>>>>> Please follow the TRM's bits naming.
>>>>>
>>>>> PMC_CNTRL_LATCHWAKE_EN
>>>>>
>>>>>>> +#define PMC_WAKE_MASK            0x0c
>>>>>>> +#define PMC_WAKE_LEVEL            0x10
>>>>>>> +#define PMC_WAKE_STATUS            0x14
>>>>>>> +#define PMC_SW_WAKE_STATUS        0x18
>>>>>>>     #define DPD_SAMPLE            0x020
>>>>>>>   #define  DPD_SAMPLE_ENABLE        BIT(0)
>>>>>>> @@ -87,6 +93,11 @@
>>>>>>>     #define PMC_SCRATCH41            0x140
>>>>>>>   +#define PMC_WAKE2_MASK            0x160
>>>>>>> +#define PMC_WAKE2_LEVEL            0x164
>>>>>>> +#define PMC_WAKE2_STATUS        0x168
>>>>>>> +#define PMC_SW_WAKE2_STATUS        0x16c
>>>>>>> +
>>>>>>>   #define PMC_SENSOR_CTRL            0x1b0
>>>>>>>   #define  PMC_SENSOR_CTRL_SCRATCH_WRITE    BIT(2)
>>>>>>>   #define  PMC_SENSOR_CTRL_ENABLE_RST    BIT(1)
>>>>>>> @@ -1922,6 +1933,55 @@ static const struct irq_domain_ops
>>>>>>> tegra_pmc_irq_domain_ops = {
>>>>>>>       .alloc = tegra_pmc_irq_alloc,
>>>>>>>   };
>>>>>>>   +static int tegra210_pmc_irq_set_wake(struct irq_data *data,
>>>>>>> unsigned int on)
>>>>>>> +{
>>>>>>> +    struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>>>>>> +    unsigned int offset, bit;
>>>>>>> +    u32 value;
>>>>>>> +
>>>>>>> +    if (data->hwirq == ULONG_MAX)
>>>>>>> +        return 0;
>>>>>>> +
>>>>>>> +    offset = data->hwirq / 32;
>>>>>>> +    bit = data->hwirq % 32;
>>>>>>> +
>>>>>>> +    /*
>>>>>>> +     * Latch wakeups to SW_WAKE_STATUS register to capture events
>>>>>>> +     * that would not make it into wakeup event register during
>>>>>>> LP0 exit.
>>>>>>> +     */
>>>>>>> +    value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>>>>>> +    value |= PMC_CNTRL_LATCH_WAKEUPS;
>>>>>>> +    tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>>>>> +    udelay(120);
>>>>>> Why it takes so much time to latch the values? Shouldn't some
>>>>>> status-bit
>>>>>> be polled for the completion of latching?
>>>>>>
>>>>>> Is this register-write really getting buffered in the PMC?
>>>>>>
>>>>>>> +    value &= ~PMC_CNTRL_LATCH_WAKEUPS;
>>>>>>> +    tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>>>>> +    udelay(120);
>>>>>> 120 usecs to remove latching, really?
>>>>>>
>>>>>>> +    tegra_pmc_writel(pmc, 0, PMC_SW_WAKE_STATUS);
>>>>>>> +    tegra_pmc_writel(pmc, 0, PMC_SW_WAKE2_STATUS);
>>>>>>> +
>>>>>>> +    tegra_pmc_writel(pmc, 0, PMC_WAKE_STATUS);
>>>>>>> +    tegra_pmc_writel(pmc, 0, PMC_WAKE2_STATUS);
>>>>>>> +
>>>>>>> +    /* enable PMC wake */
>>>>>>> +    if (data->hwirq >= 32)
>>>>>>> +        offset = PMC_WAKE2_MASK;
>>>>>>> +    else
>>>>>>> +        offset = PMC_WAKE_MASK;
>>>>>>> +
>>>>>>> +    value = tegra_pmc_readl(pmc, offset);
>>>>>>> +
>>>>>>> +    if (on)
>>>>>>> +        value |= 1 << bit;
>>>>>>> +    else
>>>>>>> +        value &= ~(1 << bit);
>>>>>>> +
>>>>>>> +    tegra_pmc_writel(pmc, value, offset);
>>>>>> Why the latching is done *before* writing into the WAKE registers?
>>>>>> What
>>>>>> it is latching then?
>>>>> I'm looking at the TRM doc and it says that latching should be done
>>>>> *after* writing to the WAKE_MASK / LEVEL registers.
>>>>>
>>>>> Secondly it says that it's enough to do:
>>>>>
>>>>> value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>>>> value |= PMC_CNTRL_LATCH_WAKEUPS;
>>>>> tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>>>
>>>>> in order to latch. There is no need for the delay and to remove the
>>>>> "LATCHWAKE_EN" bit, it should be a oneshot action.
>>>> Although, no. TRM says "stops latching on transition from 1
>>>> to 0 (sequence - set to 1,set to 0)", so it's not a oneshot action.
>>>>
>>>> Have you tested this code at all? I'm wondering how it happens to work
>>>> without a proper latching.
>>> Yes, ofcourse its tested and this sequence to do transition is
>>> recommendation from Tegra designer.
>>> Will check if TRM doesn't have update properly or will re-confirm
>>> internally on delay time...
>>>
>>> On any of the wake event PMC wakeup happens and WAKE_STATUS register
>>> will have bits set for all events that triggered wake.
>>> After wakeup PMC doesn't update SW_WAKE_STATUS register as per PMC
>>> design.
>>> SW latch register added in design helps to provide a way to capture
>>> those events that happen right during wakeup time and didnt make it to
>>> SW_WAKE_STATUS register.
>>> So before next suspend entry, latching all prior wake events into SW
>>> WAKE_STATUS and then clearing them.
>> I'm now wondering whether the latching cold be turned ON permanently
>> during of the PMC's probe, for simplicity.
> latching should be done on suspend-resume cycle as wake events gets
> generates on every suspend-resume cycle.

You're saying that PMC "doesn't update SW_WAKE_STATUS" after wake-up,
then I don't quite understand what's the point of disabling the latching
at all.

>>> LATCHWAKE_EN - When set, enables latching and stops latching on
>>> transition from 1 to 0
>>> There is recommendation of min 120uSec for this transition to stop
>>> latching. Will double-check why 120uSec
>> Yes, please check.
>>
>>>>>>> +    return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>>   static int tegra186_pmc_irq_set_wake(struct irq_data *data,
>>>>>>> unsigned int on)
>>>>>>>   {
>>>>>>>       struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>>>>>> @@ -1954,6 +2014,49 @@ static int
>>>>>>> tegra186_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
>>>>>>>       return 0;
>>>>>>>   }
>>>>>>>   +static int tegra210_pmc_irq_set_type(struct irq_data *data,
>>>>>>> unsigned int type)
>>>>>>> +{
>>>>>>> +    struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>>>>>> +    unsigned int offset, bit;
>>>>>>> +    u32 value;
>>>>>>> +
>>>>>>> +    if (data->hwirq == ULONG_MAX)
>>>>>>> +        return 0;
>>>>>>> +
>>>>>>> +    offset = data->hwirq / 32;
>>>>>>> +    bit = data->hwirq % 32;
>>>>>>> +
>>>>>>> +    if (data->hwirq >= 32)
>>>>>>> +        offset = PMC_WAKE2_LEVEL;
>>>>>>> +    else
>>>>>>> +        offset = PMC_WAKE_LEVEL;
>>>>>>> +
>>>>>>> +    value = tegra_pmc_readl(pmc, offset);
>>>>>>> +
>>>>>>> +    switch (type) {
>>>>>>> +    case IRQ_TYPE_EDGE_RISING:
>>>>>>> +    case IRQ_TYPE_LEVEL_HIGH:
>>>>>>> +        value |= 1 << bit;
>>>>>>> +        break;
>>>>>>> +
>>>>>>> +    case IRQ_TYPE_EDGE_FALLING:
>>>>>>> +    case IRQ_TYPE_LEVEL_LOW:
>>>>>>> +        value &= ~(1 << bit);
>>>>>>> +        break;
>>>>>>> +
>>>>>>> +    case IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING:
>>>>>>> +        value ^= 1 << bit;
>>>>>>> +        break;
>>>>>>> +
>>>>>>> +    default:
>>>>>>> +        return -EINVAL;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    tegra_pmc_writel(pmc, value, offset);
>>>>>> Shouldn't the WAKE_LEVEL be latched as well?
>>> WAKE_LEVELs dont need any latch as they are the levels SW sets for wake
>>> trigger and they are not status
>> Okay.
>>
>> [snip]
Sowjanya Komatineni July 23, 2019, 3:31 a.m. UTC | #9
On 7/22/19 8:25 PM, Dmitry Osipenko wrote:
> 23.07.2019 6:09, Sowjanya Komatineni пишет:
>> On 7/22/19 8:03 PM, Dmitry Osipenko wrote:
>>> 23.07.2019 4:52, Sowjanya Komatineni пишет:
>>>> On 7/22/19 6:41 PM, Dmitry Osipenko wrote:
>>>>> 23.07.2019 4:08, Dmitry Osipenko пишет:
>>>>>> 23.07.2019 3:58, Dmitry Osipenko пишет:
>>>>>>> 21.07.2019 22:40, Sowjanya Komatineni пишет:
>>>>>>>> This patch implements PMC wakeup sequence for Tegra210 and defines
>>>>>>>> common used RTC alarm wake event.
>>>>>>>>
>>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>>> ---
>>>>>>>>    drivers/soc/tegra/pmc.c | 111
>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>    1 file changed, 111 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>>>>>>>> index 91c84d0e66ae..c556f38874e1 100644
>>>>>>>> --- a/drivers/soc/tegra/pmc.c
>>>>>>>> +++ b/drivers/soc/tegra/pmc.c
>>>>>>>> @@ -57,6 +57,12 @@
>>>>>>>>    #define  PMC_CNTRL_SYSCLK_OE        BIT(11) /* system clock
>>>>>>>> enable */
>>>>>>>>    #define  PMC_CNTRL_SYSCLK_POLARITY    BIT(10) /* sys clk
>>>>>>>> polarity */
>>>>>>>>    #define  PMC_CNTRL_MAIN_RST        BIT(4)
>>>>>>>> +#define  PMC_CNTRL_LATCH_WAKEUPS    BIT(5)
>>>>>> Please follow the TRM's bits naming.
>>>>>>
>>>>>> PMC_CNTRL_LATCHWAKE_EN
>>>>>>
>>>>>>>> +#define PMC_WAKE_MASK            0x0c
>>>>>>>> +#define PMC_WAKE_LEVEL            0x10
>>>>>>>> +#define PMC_WAKE_STATUS            0x14
>>>>>>>> +#define PMC_SW_WAKE_STATUS        0x18
>>>>>>>>      #define DPD_SAMPLE            0x020
>>>>>>>>    #define  DPD_SAMPLE_ENABLE        BIT(0)
>>>>>>>> @@ -87,6 +93,11 @@
>>>>>>>>      #define PMC_SCRATCH41            0x140
>>>>>>>>    +#define PMC_WAKE2_MASK            0x160
>>>>>>>> +#define PMC_WAKE2_LEVEL            0x164
>>>>>>>> +#define PMC_WAKE2_STATUS        0x168
>>>>>>>> +#define PMC_SW_WAKE2_STATUS        0x16c
>>>>>>>> +
>>>>>>>>    #define PMC_SENSOR_CTRL            0x1b0
>>>>>>>>    #define  PMC_SENSOR_CTRL_SCRATCH_WRITE    BIT(2)
>>>>>>>>    #define  PMC_SENSOR_CTRL_ENABLE_RST    BIT(1)
>>>>>>>> @@ -1922,6 +1933,55 @@ static const struct irq_domain_ops
>>>>>>>> tegra_pmc_irq_domain_ops = {
>>>>>>>>        .alloc = tegra_pmc_irq_alloc,
>>>>>>>>    };
>>>>>>>>    +static int tegra210_pmc_irq_set_wake(struct irq_data *data,
>>>>>>>> unsigned int on)
>>>>>>>> +{
>>>>>>>> +    struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>>>>>>> +    unsigned int offset, bit;
>>>>>>>> +    u32 value;
>>>>>>>> +
>>>>>>>> +    if (data->hwirq == ULONG_MAX)
>>>>>>>> +        return 0;
>>>>>>>> +
>>>>>>>> +    offset = data->hwirq / 32;
>>>>>>>> +    bit = data->hwirq % 32;
>>>>>>>> +
>>>>>>>> +    /*
>>>>>>>> +     * Latch wakeups to SW_WAKE_STATUS register to capture events
>>>>>>>> +     * that would not make it into wakeup event register during
>>>>>>>> LP0 exit.
>>>>>>>> +     */
>>>>>>>> +    value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>>>>>>> +    value |= PMC_CNTRL_LATCH_WAKEUPS;
>>>>>>>> +    tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>>>>>> +    udelay(120);
>>>>>>> Why it takes so much time to latch the values? Shouldn't some
>>>>>>> status-bit
>>>>>>> be polled for the completion of latching?
>>>>>>>
>>>>>>> Is this register-write really getting buffered in the PMC?
>>>>>>>
>>>>>>>> +    value &= ~PMC_CNTRL_LATCH_WAKEUPS;
>>>>>>>> +    tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>>>>>> +    udelay(120);
>>>>>>> 120 usecs to remove latching, really?
>>>>>>>
>>>>>>>> +    tegra_pmc_writel(pmc, 0, PMC_SW_WAKE_STATUS);
>>>>>>>> +    tegra_pmc_writel(pmc, 0, PMC_SW_WAKE2_STATUS);
>>>>>>>> +
>>>>>>>> +    tegra_pmc_writel(pmc, 0, PMC_WAKE_STATUS);
>>>>>>>> +    tegra_pmc_writel(pmc, 0, PMC_WAKE2_STATUS);
>>>>>>>> +
>>>>>>>> +    /* enable PMC wake */
>>>>>>>> +    if (data->hwirq >= 32)
>>>>>>>> +        offset = PMC_WAKE2_MASK;
>>>>>>>> +    else
>>>>>>>> +        offset = PMC_WAKE_MASK;
>>>>>>>> +
>>>>>>>> +    value = tegra_pmc_readl(pmc, offset);
>>>>>>>> +
>>>>>>>> +    if (on)
>>>>>>>> +        value |= 1 << bit;
>>>>>>>> +    else
>>>>>>>> +        value &= ~(1 << bit);
>>>>>>>> +
>>>>>>>> +    tegra_pmc_writel(pmc, value, offset);
>>>>>>> Why the latching is done *before* writing into the WAKE registers?
>>>>>>> What
>>>>>>> it is latching then?
>>>>>> I'm looking at the TRM doc and it says that latching should be done
>>>>>> *after* writing to the WAKE_MASK / LEVEL registers.
>>>>>>
>>>>>> Secondly it says that it's enough to do:
>>>>>>
>>>>>> value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>>>>> value |= PMC_CNTRL_LATCH_WAKEUPS;
>>>>>> tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>>>>
>>>>>> in order to latch. There is no need for the delay and to remove the
>>>>>> "LATCHWAKE_EN" bit, it should be a oneshot action.
>>>>> Although, no. TRM says "stops latching on transition from 1
>>>>> to 0 (sequence - set to 1,set to 0)", so it's not a oneshot action.
>>>>>
>>>>> Have you tested this code at all? I'm wondering how it happens to work
>>>>> without a proper latching.
>>>> Yes, ofcourse its tested and this sequence to do transition is
>>>> recommendation from Tegra designer.
>>>> Will check if TRM doesn't have update properly or will re-confirm
>>>> internally on delay time...
>>>>
>>>> On any of the wake event PMC wakeup happens and WAKE_STATUS register
>>>> will have bits set for all events that triggered wake.
>>>> After wakeup PMC doesn't update SW_WAKE_STATUS register as per PMC
>>>> design.
>>>> SW latch register added in design helps to provide a way to capture
>>>> those events that happen right during wakeup time and didnt make it to
>>>> SW_WAKE_STATUS register.
>>>> So before next suspend entry, latching all prior wake events into SW
>>>> WAKE_STATUS and then clearing them.
>>> I'm now wondering whether the latching cold be turned ON permanently
>>> during of the PMC's probe, for simplicity.
>> latching should be done on suspend-resume cycle as wake events gets
>> generates on every suspend-resume cycle.
> You're saying that PMC "doesn't update SW_WAKE_STATUS" after wake-up,
> then I don't quite understand what's the point of disabling the latching
> at all.
When latch wake enable is set, events are latched and during 1 to 0 
transition latching is disabled.

This is to avoid sw_wake_status and wake_status showing diff events.

Currently driver is not relying on SW_WAKE_STATUS but its good to latch 
and clear so even at some point for some reason when SW_WAKE_STATUS is 
used, this wlil not cause mismatch with wake_status.

>>>> LATCHWAKE_EN - When set, enables latching and stops latching on
>>>> transition from 1 to 0
>>>> There is recommendation of min 120uSec for this transition to stop
>>>> latching. Will double-check why 120uSec
>>> Yes, please check.
>>>
>>>>>>>> +    return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>    static int tegra186_pmc_irq_set_wake(struct irq_data *data,
>>>>>>>> unsigned int on)
>>>>>>>>    {
>>>>>>>>        struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>>>>>>> @@ -1954,6 +2014,49 @@ static int
>>>>>>>> tegra186_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
>>>>>>>>        return 0;
>>>>>>>>    }
>>>>>>>>    +static int tegra210_pmc_irq_set_type(struct irq_data *data,
>>>>>>>> unsigned int type)
>>>>>>>> +{
>>>>>>>> +    struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>>>>>>> +    unsigned int offset, bit;
>>>>>>>> +    u32 value;
>>>>>>>> +
>>>>>>>> +    if (data->hwirq == ULONG_MAX)
>>>>>>>> +        return 0;
>>>>>>>> +
>>>>>>>> +    offset = data->hwirq / 32;
>>>>>>>> +    bit = data->hwirq % 32;
>>>>>>>> +
>>>>>>>> +    if (data->hwirq >= 32)
>>>>>>>> +        offset = PMC_WAKE2_LEVEL;
>>>>>>>> +    else
>>>>>>>> +        offset = PMC_WAKE_LEVEL;
>>>>>>>> +
>>>>>>>> +    value = tegra_pmc_readl(pmc, offset);
>>>>>>>> +
>>>>>>>> +    switch (type) {
>>>>>>>> +    case IRQ_TYPE_EDGE_RISING:
>>>>>>>> +    case IRQ_TYPE_LEVEL_HIGH:
>>>>>>>> +        value |= 1 << bit;
>>>>>>>> +        break;
>>>>>>>> +
>>>>>>>> +    case IRQ_TYPE_EDGE_FALLING:
>>>>>>>> +    case IRQ_TYPE_LEVEL_LOW:
>>>>>>>> +        value &= ~(1 << bit);
>>>>>>>> +        break;
>>>>>>>> +
>>>>>>>> +    case IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING:
>>>>>>>> +        value ^= 1 << bit;
>>>>>>>> +        break;
>>>>>>>> +
>>>>>>>> +    default:
>>>>>>>> +        return -EINVAL;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    tegra_pmc_writel(pmc, value, offset);
>>>>>>> Shouldn't the WAKE_LEVEL be latched as well?
>>>> WAKE_LEVELs dont need any latch as they are the levels SW sets for wake
>>>> trigger and they are not status
>>> Okay.
>>>
>>> [snip]
Dmitry Osipenko July 23, 2019, 3:43 a.m. UTC | #10
23.07.2019 6:31, Sowjanya Komatineni пишет:
> 
> On 7/22/19 8:25 PM, Dmitry Osipenko wrote:
>> 23.07.2019 6:09, Sowjanya Komatineni пишет:
>>> On 7/22/19 8:03 PM, Dmitry Osipenko wrote:
>>>> 23.07.2019 4:52, Sowjanya Komatineni пишет:
>>>>> On 7/22/19 6:41 PM, Dmitry Osipenko wrote:
>>>>>> 23.07.2019 4:08, Dmitry Osipenko пишет:
>>>>>>> 23.07.2019 3:58, Dmitry Osipenko пишет:
>>>>>>>> 21.07.2019 22:40, Sowjanya Komatineni пишет:
>>>>>>>>> This patch implements PMC wakeup sequence for Tegra210 and defines
>>>>>>>>> common used RTC alarm wake event.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>>>> ---
>>>>>>>>>    drivers/soc/tegra/pmc.c | 111
>>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>    1 file changed, 111 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>>>>>>>>> index 91c84d0e66ae..c556f38874e1 100644
>>>>>>>>> --- a/drivers/soc/tegra/pmc.c
>>>>>>>>> +++ b/drivers/soc/tegra/pmc.c
>>>>>>>>> @@ -57,6 +57,12 @@
>>>>>>>>>    #define  PMC_CNTRL_SYSCLK_OE        BIT(11) /* system clock
>>>>>>>>> enable */
>>>>>>>>>    #define  PMC_CNTRL_SYSCLK_POLARITY    BIT(10) /* sys clk
>>>>>>>>> polarity */
>>>>>>>>>    #define  PMC_CNTRL_MAIN_RST        BIT(4)
>>>>>>>>> +#define  PMC_CNTRL_LATCH_WAKEUPS    BIT(5)
>>>>>>> Please follow the TRM's bits naming.
>>>>>>>
>>>>>>> PMC_CNTRL_LATCHWAKE_EN
>>>>>>>
>>>>>>>>> +#define PMC_WAKE_MASK            0x0c
>>>>>>>>> +#define PMC_WAKE_LEVEL            0x10
>>>>>>>>> +#define PMC_WAKE_STATUS            0x14
>>>>>>>>> +#define PMC_SW_WAKE_STATUS        0x18
>>>>>>>>>      #define DPD_SAMPLE            0x020
>>>>>>>>>    #define  DPD_SAMPLE_ENABLE        BIT(0)
>>>>>>>>> @@ -87,6 +93,11 @@
>>>>>>>>>      #define PMC_SCRATCH41            0x140
>>>>>>>>>    +#define PMC_WAKE2_MASK            0x160
>>>>>>>>> +#define PMC_WAKE2_LEVEL            0x164
>>>>>>>>> +#define PMC_WAKE2_STATUS        0x168
>>>>>>>>> +#define PMC_SW_WAKE2_STATUS        0x16c
>>>>>>>>> +
>>>>>>>>>    #define PMC_SENSOR_CTRL            0x1b0
>>>>>>>>>    #define  PMC_SENSOR_CTRL_SCRATCH_WRITE    BIT(2)
>>>>>>>>>    #define  PMC_SENSOR_CTRL_ENABLE_RST    BIT(1)
>>>>>>>>> @@ -1922,6 +1933,55 @@ static const struct irq_domain_ops
>>>>>>>>> tegra_pmc_irq_domain_ops = {
>>>>>>>>>        .alloc = tegra_pmc_irq_alloc,
>>>>>>>>>    };
>>>>>>>>>    +static int tegra210_pmc_irq_set_wake(struct irq_data *data,
>>>>>>>>> unsigned int on)
>>>>>>>>> +{
>>>>>>>>> +    struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>>>>>>>> +    unsigned int offset, bit;
>>>>>>>>> +    u32 value;
>>>>>>>>> +
>>>>>>>>> +    if (data->hwirq == ULONG_MAX)
>>>>>>>>> +        return 0;
>>>>>>>>> +
>>>>>>>>> +    offset = data->hwirq / 32;
>>>>>>>>> +    bit = data->hwirq % 32;
>>>>>>>>> +
>>>>>>>>> +    /*
>>>>>>>>> +     * Latch wakeups to SW_WAKE_STATUS register to capture events
>>>>>>>>> +     * that would not make it into wakeup event register during
>>>>>>>>> LP0 exit.
>>>>>>>>> +     */
>>>>>>>>> +    value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>>>>>>>> +    value |= PMC_CNTRL_LATCH_WAKEUPS;
>>>>>>>>> +    tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>>>>>>> +    udelay(120);
>>>>>>>> Why it takes so much time to latch the values? Shouldn't some
>>>>>>>> status-bit
>>>>>>>> be polled for the completion of latching?
>>>>>>>>
>>>>>>>> Is this register-write really getting buffered in the PMC?
>>>>>>>>
>>>>>>>>> +    value &= ~PMC_CNTRL_LATCH_WAKEUPS;
>>>>>>>>> +    tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>>>>>>> +    udelay(120);
>>>>>>>> 120 usecs to remove latching, really?
>>>>>>>>
>>>>>>>>> +    tegra_pmc_writel(pmc, 0, PMC_SW_WAKE_STATUS);
>>>>>>>>> +    tegra_pmc_writel(pmc, 0, PMC_SW_WAKE2_STATUS);
>>>>>>>>> +
>>>>>>>>> +    tegra_pmc_writel(pmc, 0, PMC_WAKE_STATUS);
>>>>>>>>> +    tegra_pmc_writel(pmc, 0, PMC_WAKE2_STATUS);
>>>>>>>>> +
>>>>>>>>> +    /* enable PMC wake */
>>>>>>>>> +    if (data->hwirq >= 32)
>>>>>>>>> +        offset = PMC_WAKE2_MASK;
>>>>>>>>> +    else
>>>>>>>>> +        offset = PMC_WAKE_MASK;
>>>>>>>>> +
>>>>>>>>> +    value = tegra_pmc_readl(pmc, offset);
>>>>>>>>> +
>>>>>>>>> +    if (on)
>>>>>>>>> +        value |= 1 << bit;
>>>>>>>>> +    else
>>>>>>>>> +        value &= ~(1 << bit);
>>>>>>>>> +
>>>>>>>>> +    tegra_pmc_writel(pmc, value, offset);
>>>>>>>> Why the latching is done *before* writing into the WAKE registers?
>>>>>>>> What
>>>>>>>> it is latching then?
>>>>>>> I'm looking at the TRM doc and it says that latching should be done
>>>>>>> *after* writing to the WAKE_MASK / LEVEL registers.
>>>>>>>
>>>>>>> Secondly it says that it's enough to do:
>>>>>>>
>>>>>>> value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>>>>>> value |= PMC_CNTRL_LATCH_WAKEUPS;
>>>>>>> tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>>>>>
>>>>>>> in order to latch. There is no need for the delay and to remove the
>>>>>>> "LATCHWAKE_EN" bit, it should be a oneshot action.
>>>>>> Although, no. TRM says "stops latching on transition from 1
>>>>>> to 0 (sequence - set to 1,set to 0)", so it's not a oneshot action.
>>>>>>
>>>>>> Have you tested this code at all? I'm wondering how it happens to
>>>>>> work
>>>>>> without a proper latching.
>>>>> Yes, ofcourse its tested and this sequence to do transition is
>>>>> recommendation from Tegra designer.
>>>>> Will check if TRM doesn't have update properly or will re-confirm
>>>>> internally on delay time...
>>>>>
>>>>> On any of the wake event PMC wakeup happens and WAKE_STATUS register
>>>>> will have bits set for all events that triggered wake.
>>>>> After wakeup PMC doesn't update SW_WAKE_STATUS register as per PMC
>>>>> design.
>>>>> SW latch register added in design helps to provide a way to capture
>>>>> those events that happen right during wakeup time and didnt make it to
>>>>> SW_WAKE_STATUS register.
>>>>> So before next suspend entry, latching all prior wake events into SW
>>>>> WAKE_STATUS and then clearing them.
>>>> I'm now wondering whether the latching cold be turned ON permanently
>>>> during of the PMC's probe, for simplicity.
>>> latching should be done on suspend-resume cycle as wake events gets
>>> generates on every suspend-resume cycle.
>> You're saying that PMC "doesn't update SW_WAKE_STATUS" after wake-up,
>> then I don't quite understand what's the point of disabling the latching
>> at all.
> When latch wake enable is set, events are latched and during 1 to 0
> transition latching is disabled.
> 
> This is to avoid sw_wake_status and wake_status showing diff events.

Okay.

> Currently driver is not relying on SW_WAKE_STATUS but its good to latch
> and clear so even at some point for some reason when SW_WAKE_STATUS is
> used, this wlil not cause mismatch with wake_status.

Then the latching need to be enabled on suspend and disabled early on
resume to get a proper WAKE status.

[snip]
Dmitry Osipenko July 23, 2019, 2:27 p.m. UTC | #11
23.07.2019 6:43, Dmitry Osipenko пишет:
> 23.07.2019 6:31, Sowjanya Komatineni пишет:
>>
>> On 7/22/19 8:25 PM, Dmitry Osipenko wrote:
>>> 23.07.2019 6:09, Sowjanya Komatineni пишет:
>>>> On 7/22/19 8:03 PM, Dmitry Osipenko wrote:
>>>>> 23.07.2019 4:52, Sowjanya Komatineni пишет:
>>>>>> On 7/22/19 6:41 PM, Dmitry Osipenko wrote:
>>>>>>> 23.07.2019 4:08, Dmitry Osipenko пишет:
>>>>>>>> 23.07.2019 3:58, Dmitry Osipenko пишет:
>>>>>>>>> 21.07.2019 22:40, Sowjanya Komatineni пишет:
>>>>>>>>>> This patch implements PMC wakeup sequence for Tegra210 and defines
>>>>>>>>>> common used RTC alarm wake event.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>>>>> ---
>>>>>>>>>>    drivers/soc/tegra/pmc.c | 111
>>>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>>    1 file changed, 111 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>>>>>>>>>> index 91c84d0e66ae..c556f38874e1 100644
>>>>>>>>>> --- a/drivers/soc/tegra/pmc.c
>>>>>>>>>> +++ b/drivers/soc/tegra/pmc.c
>>>>>>>>>> @@ -57,6 +57,12 @@
>>>>>>>>>>    #define  PMC_CNTRL_SYSCLK_OE        BIT(11) /* system clock
>>>>>>>>>> enable */
>>>>>>>>>>    #define  PMC_CNTRL_SYSCLK_POLARITY    BIT(10) /* sys clk
>>>>>>>>>> polarity */
>>>>>>>>>>    #define  PMC_CNTRL_MAIN_RST        BIT(4)
>>>>>>>>>> +#define  PMC_CNTRL_LATCH_WAKEUPS    BIT(5)
>>>>>>>> Please follow the TRM's bits naming.
>>>>>>>>
>>>>>>>> PMC_CNTRL_LATCHWAKE_EN
>>>>>>>>
>>>>>>>>>> +#define PMC_WAKE_MASK            0x0c
>>>>>>>>>> +#define PMC_WAKE_LEVEL            0x10
>>>>>>>>>> +#define PMC_WAKE_STATUS            0x14
>>>>>>>>>> +#define PMC_SW_WAKE_STATUS        0x18
>>>>>>>>>>      #define DPD_SAMPLE            0x020
>>>>>>>>>>    #define  DPD_SAMPLE_ENABLE        BIT(0)
>>>>>>>>>> @@ -87,6 +93,11 @@
>>>>>>>>>>      #define PMC_SCRATCH41            0x140
>>>>>>>>>>    +#define PMC_WAKE2_MASK            0x160
>>>>>>>>>> +#define PMC_WAKE2_LEVEL            0x164
>>>>>>>>>> +#define PMC_WAKE2_STATUS        0x168
>>>>>>>>>> +#define PMC_SW_WAKE2_STATUS        0x16c
>>>>>>>>>> +
>>>>>>>>>>    #define PMC_SENSOR_CTRL            0x1b0
>>>>>>>>>>    #define  PMC_SENSOR_CTRL_SCRATCH_WRITE    BIT(2)
>>>>>>>>>>    #define  PMC_SENSOR_CTRL_ENABLE_RST    BIT(1)
>>>>>>>>>> @@ -1922,6 +1933,55 @@ static const struct irq_domain_ops
>>>>>>>>>> tegra_pmc_irq_domain_ops = {
>>>>>>>>>>        .alloc = tegra_pmc_irq_alloc,
>>>>>>>>>>    };
>>>>>>>>>>    +static int tegra210_pmc_irq_set_wake(struct irq_data *data,
>>>>>>>>>> unsigned int on)
>>>>>>>>>> +{
>>>>>>>>>> +    struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>>>>>>>>> +    unsigned int offset, bit;
>>>>>>>>>> +    u32 value;
>>>>>>>>>> +
>>>>>>>>>> +    if (data->hwirq == ULONG_MAX)
>>>>>>>>>> +        return 0;
>>>>>>>>>> +
>>>>>>>>>> +    offset = data->hwirq / 32;
>>>>>>>>>> +    bit = data->hwirq % 32;
>>>>>>>>>> +
>>>>>>>>>> +    /*
>>>>>>>>>> +     * Latch wakeups to SW_WAKE_STATUS register to capture events
>>>>>>>>>> +     * that would not make it into wakeup event register during
>>>>>>>>>> LP0 exit.
>>>>>>>>>> +     */
>>>>>>>>>> +    value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>>>>>>>>> +    value |= PMC_CNTRL_LATCH_WAKEUPS;
>>>>>>>>>> +    tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>>>>>>>> +    udelay(120);
>>>>>>>>> Why it takes so much time to latch the values? Shouldn't some
>>>>>>>>> status-bit
>>>>>>>>> be polled for the completion of latching?
>>>>>>>>>
>>>>>>>>> Is this register-write really getting buffered in the PMC?
>>>>>>>>>
>>>>>>>>>> +    value &= ~PMC_CNTRL_LATCH_WAKEUPS;
>>>>>>>>>> +    tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>>>>>>>> +    udelay(120);
>>>>>>>>> 120 usecs to remove latching, really?
>>>>>>>>>
>>>>>>>>>> +    tegra_pmc_writel(pmc, 0, PMC_SW_WAKE_STATUS);
>>>>>>>>>> +    tegra_pmc_writel(pmc, 0, PMC_SW_WAKE2_STATUS);
>>>>>>>>>> +
>>>>>>>>>> +    tegra_pmc_writel(pmc, 0, PMC_WAKE_STATUS);
>>>>>>>>>> +    tegra_pmc_writel(pmc, 0, PMC_WAKE2_STATUS);
>>>>>>>>>> +
>>>>>>>>>> +    /* enable PMC wake */
>>>>>>>>>> +    if (data->hwirq >= 32)
>>>>>>>>>> +        offset = PMC_WAKE2_MASK;
>>>>>>>>>> +    else
>>>>>>>>>> +        offset = PMC_WAKE_MASK;
>>>>>>>>>> +
>>>>>>>>>> +    value = tegra_pmc_readl(pmc, offset);
>>>>>>>>>> +
>>>>>>>>>> +    if (on)
>>>>>>>>>> +        value |= 1 << bit;
>>>>>>>>>> +    else
>>>>>>>>>> +        value &= ~(1 << bit);
>>>>>>>>>> +
>>>>>>>>>> +    tegra_pmc_writel(pmc, value, offset);
>>>>>>>>> Why the latching is done *before* writing into the WAKE registers?
>>>>>>>>> What
>>>>>>>>> it is latching then?
>>>>>>>> I'm looking at the TRM doc and it says that latching should be done
>>>>>>>> *after* writing to the WAKE_MASK / LEVEL registers.
>>>>>>>>
>>>>>>>> Secondly it says that it's enough to do:
>>>>>>>>
>>>>>>>> value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>>>>>>> value |= PMC_CNTRL_LATCH_WAKEUPS;
>>>>>>>> tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>>>>>>
>>>>>>>> in order to latch. There is no need for the delay and to remove the
>>>>>>>> "LATCHWAKE_EN" bit, it should be a oneshot action.
>>>>>>> Although, no. TRM says "stops latching on transition from 1
>>>>>>> to 0 (sequence - set to 1,set to 0)", so it's not a oneshot action.
>>>>>>>
>>>>>>> Have you tested this code at all? I'm wondering how it happens to
>>>>>>> work
>>>>>>> without a proper latching.
>>>>>> Yes, ofcourse its tested and this sequence to do transition is
>>>>>> recommendation from Tegra designer.
>>>>>> Will check if TRM doesn't have update properly or will re-confirm
>>>>>> internally on delay time...
>>>>>>
>>>>>> On any of the wake event PMC wakeup happens and WAKE_STATUS register
>>>>>> will have bits set for all events that triggered wake.
>>>>>> After wakeup PMC doesn't update SW_WAKE_STATUS register as per PMC
>>>>>> design.
>>>>>> SW latch register added in design helps to provide a way to capture
>>>>>> those events that happen right during wakeup time and didnt make it to
>>>>>> SW_WAKE_STATUS register.
>>>>>> So before next suspend entry, latching all prior wake events into SW
>>>>>> WAKE_STATUS and then clearing them.
>>>>> I'm now wondering whether the latching cold be turned ON permanently
>>>>> during of the PMC's probe, for simplicity.
>>>> latching should be done on suspend-resume cycle as wake events gets
>>>> generates on every suspend-resume cycle.
>>> You're saying that PMC "doesn't update SW_WAKE_STATUS" after wake-up,
>>> then I don't quite understand what's the point of disabling the latching
>>> at all.
>> When latch wake enable is set, events are latched and during 1 to 0
>> transition latching is disabled.
>>
>> This is to avoid sw_wake_status and wake_status showing diff events.
> 
> Okay.
> 
>> Currently driver is not relying on SW_WAKE_STATUS but its good to latch
>> and clear so even at some point for some reason when SW_WAKE_STATUS is
>> used, this wlil not cause mismatch with wake_status.
> 
> Then the latching need to be enabled on suspend and disabled early on
> resume to get a proper WAKE status.

Actually, it will be better to simply not implement the latching until
it will become really needed. In general you shouldn't add into the
patchset anything that is unused.
Sowjanya Komatineni July 23, 2019, 11:39 p.m. UTC | #12
On 7/23/19 7:27 AM, Dmitry Osipenko wrote:
> 23.07.2019 6:43, Dmitry Osipenko пишет:
>> 23.07.2019 6:31, Sowjanya Komatineni пишет:
>>> On 7/22/19 8:25 PM, Dmitry Osipenko wrote:
>>>> 23.07.2019 6:09, Sowjanya Komatineni пишет:
>>>>> On 7/22/19 8:03 PM, Dmitry Osipenko wrote:
>>>>>> 23.07.2019 4:52, Sowjanya Komatineni пишет:
>>>>>>> On 7/22/19 6:41 PM, Dmitry Osipenko wrote:
>>>>>>>> 23.07.2019 4:08, Dmitry Osipenko пишет:
>>>>>>>>> 23.07.2019 3:58, Dmitry Osipenko пишет:
>>>>>>>>>> 21.07.2019 22:40, Sowjanya Komatineni пишет:
>>>>>>>>>>> This patch implements PMC wakeup sequence for Tegra210 and defines
>>>>>>>>>>> common used RTC alarm wake event.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>>>>>> ---
>>>>>>>>>>>     drivers/soc/tegra/pmc.c | 111
>>>>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>     1 file changed, 111 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>>>>>>>>>>> index 91c84d0e66ae..c556f38874e1 100644
>>>>>>>>>>> --- a/drivers/soc/tegra/pmc.c
>>>>>>>>>>> +++ b/drivers/soc/tegra/pmc.c
>>>>>>>>>>> @@ -57,6 +57,12 @@
>>>>>>>>>>>     #define  PMC_CNTRL_SYSCLK_OE        BIT(11) /* system clock
>>>>>>>>>>> enable */
>>>>>>>>>>>     #define  PMC_CNTRL_SYSCLK_POLARITY    BIT(10) /* sys clk
>>>>>>>>>>> polarity */
>>>>>>>>>>>     #define  PMC_CNTRL_MAIN_RST        BIT(4)
>>>>>>>>>>> +#define  PMC_CNTRL_LATCH_WAKEUPS    BIT(5)
>>>>>>>>> Please follow the TRM's bits naming.
>>>>>>>>>
>>>>>>>>> PMC_CNTRL_LATCHWAKE_EN
>>>>>>>>>
>>>>>>>>>>> +#define PMC_WAKE_MASK            0x0c
>>>>>>>>>>> +#define PMC_WAKE_LEVEL            0x10
>>>>>>>>>>> +#define PMC_WAKE_STATUS            0x14
>>>>>>>>>>> +#define PMC_SW_WAKE_STATUS        0x18
>>>>>>>>>>>       #define DPD_SAMPLE            0x020
>>>>>>>>>>>     #define  DPD_SAMPLE_ENABLE        BIT(0)
>>>>>>>>>>> @@ -87,6 +93,11 @@
>>>>>>>>>>>       #define PMC_SCRATCH41            0x140
>>>>>>>>>>>     +#define PMC_WAKE2_MASK            0x160
>>>>>>>>>>> +#define PMC_WAKE2_LEVEL            0x164
>>>>>>>>>>> +#define PMC_WAKE2_STATUS        0x168
>>>>>>>>>>> +#define PMC_SW_WAKE2_STATUS        0x16c
>>>>>>>>>>> +
>>>>>>>>>>>     #define PMC_SENSOR_CTRL            0x1b0
>>>>>>>>>>>     #define  PMC_SENSOR_CTRL_SCRATCH_WRITE    BIT(2)
>>>>>>>>>>>     #define  PMC_SENSOR_CTRL_ENABLE_RST    BIT(1)
>>>>>>>>>>> @@ -1922,6 +1933,55 @@ static const struct irq_domain_ops
>>>>>>>>>>> tegra_pmc_irq_domain_ops = {
>>>>>>>>>>>         .alloc = tegra_pmc_irq_alloc,
>>>>>>>>>>>     };
>>>>>>>>>>>     +static int tegra210_pmc_irq_set_wake(struct irq_data *data,
>>>>>>>>>>> unsigned int on)
>>>>>>>>>>> +{
>>>>>>>>>>> +    struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>>>>>>>>>> +    unsigned int offset, bit;
>>>>>>>>>>> +    u32 value;
>>>>>>>>>>> +
>>>>>>>>>>> +    if (data->hwirq == ULONG_MAX)
>>>>>>>>>>> +        return 0;
>>>>>>>>>>> +
>>>>>>>>>>> +    offset = data->hwirq / 32;
>>>>>>>>>>> +    bit = data->hwirq % 32;
>>>>>>>>>>> +
>>>>>>>>>>> +    /*
>>>>>>>>>>> +     * Latch wakeups to SW_WAKE_STATUS register to capture events
>>>>>>>>>>> +     * that would not make it into wakeup event register during
>>>>>>>>>>> LP0 exit.
>>>>>>>>>>> +     */
>>>>>>>>>>> +    value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>>>>>>>>>> +    value |= PMC_CNTRL_LATCH_WAKEUPS;
>>>>>>>>>>> +    tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>>>>>>>>> +    udelay(120);
>>>>>>>>>> Why it takes so much time to latch the values? Shouldn't some
>>>>>>>>>> status-bit
>>>>>>>>>> be polled for the completion of latching?
>>>>>>>>>>
>>>>>>>>>> Is this register-write really getting buffered in the PMC?
>>>>>>>>>>
>>>>>>>>>>> +    value &= ~PMC_CNTRL_LATCH_WAKEUPS;
>>>>>>>>>>> +    tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>>>>>>>>> +    udelay(120);
>>>>>>>>>> 120 usecs to remove latching, really?
>>>>>>>>>>
>>>>>>>>>>> +    tegra_pmc_writel(pmc, 0, PMC_SW_WAKE_STATUS);
>>>>>>>>>>> +    tegra_pmc_writel(pmc, 0, PMC_SW_WAKE2_STATUS);
>>>>>>>>>>> +
>>>>>>>>>>> +    tegra_pmc_writel(pmc, 0, PMC_WAKE_STATUS);
>>>>>>>>>>> +    tegra_pmc_writel(pmc, 0, PMC_WAKE2_STATUS);
>>>>>>>>>>> +
>>>>>>>>>>> +    /* enable PMC wake */
>>>>>>>>>>> +    if (data->hwirq >= 32)
>>>>>>>>>>> +        offset = PMC_WAKE2_MASK;
>>>>>>>>>>> +    else
>>>>>>>>>>> +        offset = PMC_WAKE_MASK;
>>>>>>>>>>> +
>>>>>>>>>>> +    value = tegra_pmc_readl(pmc, offset);
>>>>>>>>>>> +
>>>>>>>>>>> +    if (on)
>>>>>>>>>>> +        value |= 1 << bit;
>>>>>>>>>>> +    else
>>>>>>>>>>> +        value &= ~(1 << bit);
>>>>>>>>>>> +
>>>>>>>>>>> +    tegra_pmc_writel(pmc, value, offset);
>>>>>>>>>> Why the latching is done *before* writing into the WAKE registers?
>>>>>>>>>> What
>>>>>>>>>> it is latching then?
>>>>>>>>> I'm looking at the TRM doc and it says that latching should be done
>>>>>>>>> *after* writing to the WAKE_MASK / LEVEL registers.
>>>>>>>>>
>>>>>>>>> Secondly it says that it's enough to do:
>>>>>>>>>
>>>>>>>>> value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>>>>>>>> value |= PMC_CNTRL_LATCH_WAKEUPS;
>>>>>>>>> tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>>>>>>>
>>>>>>>>> in order to latch. There is no need for the delay and to remove the
>>>>>>>>> "LATCHWAKE_EN" bit, it should be a oneshot action.
>>>>>>>> Although, no. TRM says "stops latching on transition from 1
>>>>>>>> to 0 (sequence - set to 1,set to 0)", so it's not a oneshot action.
>>>>>>>>
>>>>>>>> Have you tested this code at all? I'm wondering how it happens to
>>>>>>>> work
>>>>>>>> without a proper latching.
>>>>>>> Yes, ofcourse its tested and this sequence to do transition is
>>>>>>> recommendation from Tegra designer.
>>>>>>> Will check if TRM doesn't have update properly or will re-confirm
>>>>>>> internally on delay time...
>>>>>>>
>>>>>>> On any of the wake event PMC wakeup happens and WAKE_STATUS register
>>>>>>> will have bits set for all events that triggered wake.
>>>>>>> After wakeup PMC doesn't update SW_WAKE_STATUS register as per PMC
>>>>>>> design.
>>>>>>> SW latch register added in design helps to provide a way to capture
>>>>>>> those events that happen right during wakeup time and didnt make it to
>>>>>>> SW_WAKE_STATUS register.
>>>>>>> So before next suspend entry, latching all prior wake events into SW
>>>>>>> WAKE_STATUS and then clearing them.
>>>>>> I'm now wondering whether the latching cold be turned ON permanently
>>>>>> during of the PMC's probe, for simplicity.
>>>>> latching should be done on suspend-resume cycle as wake events gets
>>>>> generates on every suspend-resume cycle.
>>>> You're saying that PMC "doesn't update SW_WAKE_STATUS" after wake-up,
>>>> then I don't quite understand what's the point of disabling the latching
>>>> at all.
>>> When latch wake enable is set, events are latched and during 1 to 0
>>> transition latching is disabled.
>>>
>>> This is to avoid sw_wake_status and wake_status showing diff events.
>> Okay.
>>
>>> Currently driver is not relying on SW_WAKE_STATUS but its good to latch
>>> and clear so even at some point for some reason when SW_WAKE_STATUS is
>>> used, this wlil not cause mismatch with wake_status.
>> Then the latching need to be enabled on suspend and disabled early on
>> resume to get a proper WAKE status.
> Actually, it will be better to simply not implement the latching until
> it will become really needed. In general you shouldn't add into the
> patchset anything that is unused.

OK, will remove latch_wake for now.

Will send next version once I get all the review feedback ..
Dmitry Osipenko July 24, 2019, 9:31 a.m. UTC | #13
24.07.2019 2:39, Sowjanya Komatineni пишет:
> 
> On 7/23/19 7:27 AM, Dmitry Osipenko wrote:
>> 23.07.2019 6:43, Dmitry Osipenko пишет:
>>> 23.07.2019 6:31, Sowjanya Komatineni пишет:
>>>> On 7/22/19 8:25 PM, Dmitry Osipenko wrote:
>>>>> 23.07.2019 6:09, Sowjanya Komatineni пишет:
>>>>>> On 7/22/19 8:03 PM, Dmitry Osipenko wrote:
>>>>>>> 23.07.2019 4:52, Sowjanya Komatineni пишет:
>>>>>>>> On 7/22/19 6:41 PM, Dmitry Osipenko wrote:
>>>>>>>>> 23.07.2019 4:08, Dmitry Osipenko пишет:
>>>>>>>>>> 23.07.2019 3:58, Dmitry Osipenko пишет:
>>>>>>>>>>> 21.07.2019 22:40, Sowjanya Komatineni пишет:
>>>>>>>>>>>> This patch implements PMC wakeup sequence for Tegra210 and
>>>>>>>>>>>> defines
>>>>>>>>>>>> common used RTC alarm wake event.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>>     drivers/soc/tegra/pmc.c | 111
>>>>>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>>     1 file changed, 111 insertions(+)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>>>>>>>>>>>> index 91c84d0e66ae..c556f38874e1 100644
>>>>>>>>>>>> --- a/drivers/soc/tegra/pmc.c
>>>>>>>>>>>> +++ b/drivers/soc/tegra/pmc.c
>>>>>>>>>>>> @@ -57,6 +57,12 @@
>>>>>>>>>>>>     #define  PMC_CNTRL_SYSCLK_OE        BIT(11) /* system clock
>>>>>>>>>>>> enable */
>>>>>>>>>>>>     #define  PMC_CNTRL_SYSCLK_POLARITY    BIT(10) /* sys clk
>>>>>>>>>>>> polarity */
>>>>>>>>>>>>     #define  PMC_CNTRL_MAIN_RST        BIT(4)
>>>>>>>>>>>> +#define  PMC_CNTRL_LATCH_WAKEUPS    BIT(5)
>>>>>>>>>> Please follow the TRM's bits naming.
>>>>>>>>>>
>>>>>>>>>> PMC_CNTRL_LATCHWAKE_EN
>>>>>>>>>>
>>>>>>>>>>>> +#define PMC_WAKE_MASK            0x0c
>>>>>>>>>>>> +#define PMC_WAKE_LEVEL            0x10
>>>>>>>>>>>> +#define PMC_WAKE_STATUS            0x14
>>>>>>>>>>>> +#define PMC_SW_WAKE_STATUS        0x18
>>>>>>>>>>>>       #define DPD_SAMPLE            0x020
>>>>>>>>>>>>     #define  DPD_SAMPLE_ENABLE        BIT(0)
>>>>>>>>>>>> @@ -87,6 +93,11 @@
>>>>>>>>>>>>       #define PMC_SCRATCH41            0x140
>>>>>>>>>>>>     +#define PMC_WAKE2_MASK            0x160
>>>>>>>>>>>> +#define PMC_WAKE2_LEVEL            0x164
>>>>>>>>>>>> +#define PMC_WAKE2_STATUS        0x168
>>>>>>>>>>>> +#define PMC_SW_WAKE2_STATUS        0x16c
>>>>>>>>>>>> +
>>>>>>>>>>>>     #define PMC_SENSOR_CTRL            0x1b0
>>>>>>>>>>>>     #define  PMC_SENSOR_CTRL_SCRATCH_WRITE    BIT(2)
>>>>>>>>>>>>     #define  PMC_SENSOR_CTRL_ENABLE_RST    BIT(1)
>>>>>>>>>>>> @@ -1922,6 +1933,55 @@ static const struct irq_domain_ops
>>>>>>>>>>>> tegra_pmc_irq_domain_ops = {
>>>>>>>>>>>>         .alloc = tegra_pmc_irq_alloc,
>>>>>>>>>>>>     };
>>>>>>>>>>>>     +static int tegra210_pmc_irq_set_wake(struct irq_data
>>>>>>>>>>>> *data,
>>>>>>>>>>>> unsigned int on)
>>>>>>>>>>>> +{
>>>>>>>>>>>> +    struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>>>>>>>>>>>> +    unsigned int offset, bit;
>>>>>>>>>>>> +    u32 value;
>>>>>>>>>>>> +
>>>>>>>>>>>> +    if (data->hwirq == ULONG_MAX)
>>>>>>>>>>>> +        return 0;
>>>>>>>>>>>> +
>>>>>>>>>>>> +    offset = data->hwirq / 32;
>>>>>>>>>>>> +    bit = data->hwirq % 32;
>>>>>>>>>>>> +
>>>>>>>>>>>> +    /*
>>>>>>>>>>>> +     * Latch wakeups to SW_WAKE_STATUS register to capture
>>>>>>>>>>>> events
>>>>>>>>>>>> +     * that would not make it into wakeup event register
>>>>>>>>>>>> during
>>>>>>>>>>>> LP0 exit.
>>>>>>>>>>>> +     */
>>>>>>>>>>>> +    value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>>>>>>>>>>> +    value |= PMC_CNTRL_LATCH_WAKEUPS;
>>>>>>>>>>>> +    tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>>>>>>>>>> +    udelay(120);
>>>>>>>>>>> Why it takes so much time to latch the values? Shouldn't some
>>>>>>>>>>> status-bit
>>>>>>>>>>> be polled for the completion of latching?
>>>>>>>>>>>
>>>>>>>>>>> Is this register-write really getting buffered in the PMC?
>>>>>>>>>>>
>>>>>>>>>>>> +    value &= ~PMC_CNTRL_LATCH_WAKEUPS;
>>>>>>>>>>>> +    tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>>>>>>>>>> +    udelay(120);
>>>>>>>>>>> 120 usecs to remove latching, really?
>>>>>>>>>>>
>>>>>>>>>>>> +    tegra_pmc_writel(pmc, 0, PMC_SW_WAKE_STATUS);
>>>>>>>>>>>> +    tegra_pmc_writel(pmc, 0, PMC_SW_WAKE2_STATUS);
>>>>>>>>>>>> +
>>>>>>>>>>>> +    tegra_pmc_writel(pmc, 0, PMC_WAKE_STATUS);
>>>>>>>>>>>> +    tegra_pmc_writel(pmc, 0, PMC_WAKE2_STATUS);
>>>>>>>>>>>> +
>>>>>>>>>>>> +    /* enable PMC wake */
>>>>>>>>>>>> +    if (data->hwirq >= 32)
>>>>>>>>>>>> +        offset = PMC_WAKE2_MASK;
>>>>>>>>>>>> +    else
>>>>>>>>>>>> +        offset = PMC_WAKE_MASK;
>>>>>>>>>>>> +
>>>>>>>>>>>> +    value = tegra_pmc_readl(pmc, offset);
>>>>>>>>>>>> +
>>>>>>>>>>>> +    if (on)
>>>>>>>>>>>> +        value |= 1 << bit;
>>>>>>>>>>>> +    else
>>>>>>>>>>>> +        value &= ~(1 << bit);
>>>>>>>>>>>> +
>>>>>>>>>>>> +    tegra_pmc_writel(pmc, value, offset);
>>>>>>>>>>> Why the latching is done *before* writing into the WAKE
>>>>>>>>>>> registers?
>>>>>>>>>>> What
>>>>>>>>>>> it is latching then?
>>>>>>>>>> I'm looking at the TRM doc and it says that latching should be
>>>>>>>>>> done
>>>>>>>>>> *after* writing to the WAKE_MASK / LEVEL registers.
>>>>>>>>>>
>>>>>>>>>> Secondly it says that it's enough to do:
>>>>>>>>>>
>>>>>>>>>> value = tegra_pmc_readl(pmc, PMC_CNTRL);
>>>>>>>>>> value |= PMC_CNTRL_LATCH_WAKEUPS;
>>>>>>>>>> tegra_pmc_writel(pmc, value, PMC_CNTRL);
>>>>>>>>>>
>>>>>>>>>> in order to latch. There is no need for the delay and to
>>>>>>>>>> remove the
>>>>>>>>>> "LATCHWAKE_EN" bit, it should be a oneshot action.
>>>>>>>>> Although, no. TRM says "stops latching on transition from 1
>>>>>>>>> to 0 (sequence - set to 1,set to 0)", so it's not a oneshot
>>>>>>>>> action.
>>>>>>>>>
>>>>>>>>> Have you tested this code at all? I'm wondering how it happens to
>>>>>>>>> work
>>>>>>>>> without a proper latching.
>>>>>>>> Yes, ofcourse its tested and this sequence to do transition is
>>>>>>>> recommendation from Tegra designer.
>>>>>>>> Will check if TRM doesn't have update properly or will re-confirm
>>>>>>>> internally on delay time...
>>>>>>>>
>>>>>>>> On any of the wake event PMC wakeup happens and WAKE_STATUS
>>>>>>>> register
>>>>>>>> will have bits set for all events that triggered wake.
>>>>>>>> After wakeup PMC doesn't update SW_WAKE_STATUS register as per PMC
>>>>>>>> design.
>>>>>>>> SW latch register added in design helps to provide a way to capture
>>>>>>>> those events that happen right during wakeup time and didnt make
>>>>>>>> it to
>>>>>>>> SW_WAKE_STATUS register.
>>>>>>>> So before next suspend entry, latching all prior wake events
>>>>>>>> into SW
>>>>>>>> WAKE_STATUS and then clearing them.
>>>>>>> I'm now wondering whether the latching cold be turned ON permanently
>>>>>>> during of the PMC's probe, for simplicity.
>>>>>> latching should be done on suspend-resume cycle as wake events gets
>>>>>> generates on every suspend-resume cycle.
>>>>> You're saying that PMC "doesn't update SW_WAKE_STATUS" after wake-up,
>>>>> then I don't quite understand what's the point of disabling the
>>>>> latching
>>>>> at all.
>>>> When latch wake enable is set, events are latched and during 1 to 0
>>>> transition latching is disabled.
>>>>
>>>> This is to avoid sw_wake_status and wake_status showing diff events.
>>> Okay.
>>>
>>>> Currently driver is not relying on SW_WAKE_STATUS but its good to latch
>>>> and clear so even at some point for some reason when SW_WAKE_STATUS is
>>>> used, this wlil not cause mismatch with wake_status.
>>> Then the latching need to be enabled on suspend and disabled early on
>>> resume to get a proper WAKE status.
>> Actually, it will be better to simply not implement the latching until
>> it will become really needed. In general you shouldn't add into the
>> patchset anything that is unused.
> 
> OK, will remove latch_wake for now.
> 
> Will send next version once I get all the review feedback ..
> 

That's not a bad idea. Wait for one-two weeks and if it will happen that
nobody is replying, then just issue a new version.
diff mbox series

Patch

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 91c84d0e66ae..c556f38874e1 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -57,6 +57,12 @@ 
 #define  PMC_CNTRL_SYSCLK_OE		BIT(11) /* system clock enable */
 #define  PMC_CNTRL_SYSCLK_POLARITY	BIT(10) /* sys clk polarity */
 #define  PMC_CNTRL_MAIN_RST		BIT(4)
+#define  PMC_CNTRL_LATCH_WAKEUPS	BIT(5)
+
+#define PMC_WAKE_MASK			0x0c
+#define PMC_WAKE_LEVEL			0x10
+#define PMC_WAKE_STATUS			0x14
+#define PMC_SW_WAKE_STATUS		0x18
 
 #define DPD_SAMPLE			0x020
 #define  DPD_SAMPLE_ENABLE		BIT(0)
@@ -87,6 +93,11 @@ 
 
 #define PMC_SCRATCH41			0x140
 
+#define PMC_WAKE2_MASK			0x160
+#define PMC_WAKE2_LEVEL			0x164
+#define PMC_WAKE2_STATUS		0x168
+#define PMC_SW_WAKE2_STATUS		0x16c
+
 #define PMC_SENSOR_CTRL			0x1b0
 #define  PMC_SENSOR_CTRL_SCRATCH_WRITE	BIT(2)
 #define  PMC_SENSOR_CTRL_ENABLE_RST	BIT(1)
@@ -1922,6 +1933,55 @@  static const struct irq_domain_ops tegra_pmc_irq_domain_ops = {
 	.alloc = tegra_pmc_irq_alloc,
 };
 
+static int tegra210_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
+{
+	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
+	unsigned int offset, bit;
+	u32 value;
+
+	if (data->hwirq == ULONG_MAX)
+		return 0;
+
+	offset = data->hwirq / 32;
+	bit = data->hwirq % 32;
+
+	/*
+	 * Latch wakeups to SW_WAKE_STATUS register to capture events
+	 * that would not make it into wakeup event register during LP0 exit.
+	 */
+	value = tegra_pmc_readl(pmc, PMC_CNTRL);
+	value |= PMC_CNTRL_LATCH_WAKEUPS;
+	tegra_pmc_writel(pmc, value, PMC_CNTRL);
+	udelay(120);
+
+	value &= ~PMC_CNTRL_LATCH_WAKEUPS;
+	tegra_pmc_writel(pmc, value, PMC_CNTRL);
+	udelay(120);
+
+	tegra_pmc_writel(pmc, 0, PMC_SW_WAKE_STATUS);
+	tegra_pmc_writel(pmc, 0, PMC_SW_WAKE2_STATUS);
+
+	tegra_pmc_writel(pmc, 0, PMC_WAKE_STATUS);
+	tegra_pmc_writel(pmc, 0, PMC_WAKE2_STATUS);
+
+	/* enable PMC wake */
+	if (data->hwirq >= 32)
+		offset = PMC_WAKE2_MASK;
+	else
+		offset = PMC_WAKE_MASK;
+
+	value = tegra_pmc_readl(pmc, offset);
+
+	if (on)
+		value |= 1 << bit;
+	else
+		value &= ~(1 << bit);
+
+	tegra_pmc_writel(pmc, value, offset);
+
+	return 0;
+}
+
 static int tegra186_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
 {
 	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
@@ -1954,6 +2014,49 @@  static int tegra186_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
 	return 0;
 }
 
+static int tegra210_pmc_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
+	unsigned int offset, bit;
+	u32 value;
+
+	if (data->hwirq == ULONG_MAX)
+		return 0;
+
+	offset = data->hwirq / 32;
+	bit = data->hwirq % 32;
+
+	if (data->hwirq >= 32)
+		offset = PMC_WAKE2_LEVEL;
+	else
+		offset = PMC_WAKE_LEVEL;
+
+	value = tegra_pmc_readl(pmc, offset);
+
+	switch (type) {
+	case IRQ_TYPE_EDGE_RISING:
+	case IRQ_TYPE_LEVEL_HIGH:
+		value |= 1 << bit;
+		break;
+
+	case IRQ_TYPE_EDGE_FALLING:
+	case IRQ_TYPE_LEVEL_LOW:
+		value &= ~(1 << bit);
+		break;
+
+	case IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING:
+		value ^= 1 << bit;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	tegra_pmc_writel(pmc, value, offset);
+
+	return 0;
+}
+
 static int tegra186_pmc_irq_set_type(struct irq_data *data, unsigned int type)
 {
 	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
@@ -2540,6 +2643,10 @@  static const struct pinctrl_pin_desc tegra210_pin_descs[] = {
 	TEGRA210_IO_PAD_TABLE(TEGRA_IO_PIN_DESC)
 };
 
+static const struct tegra_wake_event tegra210_wake_events[] = {
+	TEGRA_WAKE_IRQ("rtc", 16, 2),
+};
+
 static const struct tegra_pmc_soc tegra210_pmc_soc = {
 	.num_powergates = ARRAY_SIZE(tegra210_powergates),
 	.powergates = tegra210_powergates,
@@ -2557,10 +2664,14 @@  static const struct tegra_pmc_soc tegra210_pmc_soc = {
 	.regs = &tegra20_pmc_regs,
 	.init = tegra20_pmc_init,
 	.setup_irq_polarity = tegra20_pmc_setup_irq_polarity,
+	.irq_set_wake = tegra210_pmc_irq_set_wake,
+	.irq_set_type = tegra210_pmc_irq_set_type,
 	.reset_sources = tegra210_reset_sources,
 	.num_reset_sources = ARRAY_SIZE(tegra210_reset_sources),
 	.reset_levels = NULL,
 	.num_reset_levels = 0,
+	.num_wake_events = ARRAY_SIZE(tegra210_wake_events),
+	.wake_events = tegra210_wake_events,
 };
 
 #define TEGRA186_IO_PAD_TABLE(_pad)					     \