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

[Dmitry Osipenko]

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@xxxxxxxxxx>
>>>>>>>>>> ---
>>>>>>>>>> ÂÂ 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.