From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
To: Ryan.Wanner@microchip.com, lee@kernel.org, robh@kernel.org,
krzk+dt@kernel.org, conor+dt@kernel.org, sre@kernel.org,
Nicolas.Ferre@microchip.com, alexandre.belloni@bootlin.com,
p.zabel@pengutronix.de
Cc: linux@armlinux.org.uk, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, linux-rtc@vger.kernel.org
Subject: Re: [PATCH v2 12/15] ARM: at91: pm: Enable ULP0 for SAMA7D65
Date: Mon, 24 Feb 2025 10:55:23 +0200 [thread overview]
Message-ID: <edb77ae7-d837-4e30-99ec-9ecb8b0647e9@tuxon.dev> (raw)
In-Reply-To: <b03c0871-a846-43a1-a4e2-d8d9ee8ef078@microchip.com>
On 19.02.2025 17:24, Ryan.Wanner@microchip.com wrote:
> On 2/17/25 00:18, Claudiu Beznea wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> Hi, Ryan,
>>
>> On 14.02.2025 20:09, Ryan.Wanner@microchip.com wrote:
>>> On 2/13/25 01:20, Claudiu Beznea wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> Hi, Ryan,
>>>>
>>>>
>>>> On 10.02.2025 23:13, Ryan.Wanner@microchip.com wrote:
>>>>> From: Ryan Wanner <Ryan.Wanner@microchip.com>
>>>>>
>>>>> New clocks are saved to enable ULP0 for SAMA7D65 because this SoC has a
>>>>> total of 10 main clocks that need to be saved for ULP0 mode.
>>>>
>>>> Isn't 9 the total number of MCKs that are handled in the last/first phase
>>>> of suspend/resume?
>>> Yes I was including 10 to match the indexing in the mck_count variable.
>>> Since bgt instruction was suggested I will correct this to reflect the
>>> true behavior of the change.
>>>>
>>>> Also, the state of MCKs are saved/restored for ULP0 and ULP1 as well.
>>>>
>>>>>
>>>>> Add mck_count member to at91_pm_data, this will be used to determine
>>>>> how many mcks need to be saved. In the mck_count member will also make
>>>>> sure that no unnecessary clock settings are written during
>>>>> mck_ps_restore.
>>>>>
>>>>> Add SHDWC to ULP0 mapping to clear the SHDWC status after exiting low
>>>>> power modes.
>>>>
>>>> Can you explain why this clear need to be done? The commit message should
>>>> answer to the "what?" and "why?" questions.
>>>>
>>>>>
>>>>> Signed-off-by: Ryan Wanner <Ryan.Wanner@microchip.com>
>>>>> Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
>>>>> ---
>>>>> arch/arm/mach-at91/pm.c | 19 +++++-
>>>>> arch/arm/mach-at91/pm.h | 1 +
>>>>> arch/arm/mach-at91/pm_data-offsets.c | 2 +
>>>>> arch/arm/mach-at91/pm_suspend.S | 97 ++++++++++++++++++++++++++--
>>>>> 4 files changed, 110 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
>>>>> index 55cab31ce1ecb..50bada544eede 100644
>>>>> --- a/arch/arm/mach-at91/pm.c
>>>>> +++ b/arch/arm/mach-at91/pm.c
>>>>> @@ -1337,6 +1337,7 @@ struct pmc_info {
>>>>> unsigned long uhp_udp_mask;
>>>>> unsigned long mckr;
>>>>> unsigned long version;
>>>>> + unsigned long mck_count;> };
>>>>>
>>>>> static const struct pmc_info pmc_infos[] __initconst = {
>>>>> @@ -1344,30 +1345,42 @@ static const struct pmc_info pmc_infos[] __initconst = {
>>>>> .uhp_udp_mask = AT91RM9200_PMC_UHP | AT91RM9200_PMC_UDP,
>>>>> .mckr = 0x30,
>>>>> .version = AT91_PMC_V1,
>>>>> + .mck_count = 1,
>>>>
>>>> As this member is used only for SAMA7 SoCs I would drop it here and above
>>>> (where initialized with 1).
>>>>
>>>>> },
>>>>>
>>>>> {
>>>>> .uhp_udp_mask = AT91SAM926x_PMC_UHP | AT91SAM926x_PMC_UDP,
>>>>> .mckr = 0x30,
>>>>> .version = AT91_PMC_V1,
>>>>> + .mck_count = 1,
>>>>> },
>>>>> {
>>>>> .uhp_udp_mask = AT91SAM926x_PMC_UHP,
>>>>> .mckr = 0x30,
>>>>> .version = AT91_PMC_V1,
>>>>> + .mck_count = 1,
>>>>> },
>>>>> { .uhp_udp_mask = 0,
>>>>> .mckr = 0x30,
>>>>> .version = AT91_PMC_V1,
>>>>> + .mck_count = 1,
>>>>> },
>>>>> {
>>>>> .uhp_udp_mask = AT91SAM926x_PMC_UHP | AT91SAM926x_PMC_UDP,
>>>>> .mckr = 0x28,
>>>>> .version = AT91_PMC_V2,
>>>>> + .mck_count = 1,
>>>>> },
>>>>> {
>>>>> .mckr = 0x28,
>>>>> .version = AT91_PMC_V2,
>>>>> + .mck_count = 5,
>>>>
>>>> I'm not sure mck_count is a good name when used like proposed in this
>>>> patch. We know that only 4 MCKs need to be handled for SAMA7G5 and 9 for
>>>> SAMA7D65.
>>>>
>>>> Maybe, better change it here to 4 (.mck_count = 4) and to 9 above
>>>> (.mck_count = 9) and adjust properly the assembly macros (see below)? What
>>>> do you think?
>>>
>>> Yes I think this is better and cleaner to read. Should this mck_count
>>> match the pmc_mck_count variable name? Or should this be more
>>> descriptive or would mcks be sufficient.
>>
>> mck_count/mcks should be enough. These will be anyway in the context of
>> pmc_info.
>>
>>>>
>>>>> + },
>>>>> + {
>>>>> + .uhp_udp_mask = AT91SAM926x_PMC_UHP,
>>>>> + .mckr = 0x28,
>>>>> + .version = AT91_PMC_V2,
>>>>> + .mck_count = 10,
>>>>> },
>>>>>
>>>>> };
>>>>> @@ -1386,7 +1399,7 @@ static const struct of_device_id atmel_pmc_ids[] __initconst = {
>>>>> { .compatible = "atmel,sama5d2-pmc", .data = &pmc_infos[1] },
>>>>> { .compatible = "microchip,sam9x60-pmc", .data = &pmc_infos[4] },
>>>>> { .compatible = "microchip,sam9x7-pmc", .data = &pmc_infos[4] },
>>>>> - { .compatible = "microchip,sama7d65-pmc", .data = &pmc_infos[4] },
>>>>> + { .compatible = "microchip,sama7d65-pmc", .data = &pmc_infos[6] },
>>>>> { .compatible = "microchip,sama7g5-pmc", .data = &pmc_infos[5] },
>>>>> { /* sentinel */ },
>>>>> };
>>>>> @@ -1457,6 +1470,7 @@ static void __init at91_pm_init(void (*pm_idle)(void))
>>>>> soc_pm.data.uhp_udp_mask = pmc->uhp_udp_mask;
>>>>> soc_pm.data.pmc_mckr_offset = pmc->mckr;
>>>>> soc_pm.data.pmc_version = pmc->version;
>>>>> + soc_pm.data.pmc_mck_count = pmc->mck_count;
>>>>>
>>>>> if (pm_idle)
>>>>> arm_pm_idle = pm_idle;
>>>>> @@ -1659,7 +1673,8 @@ void __init sama7_pm_init(void)
>>>>> AT91_PM_STANDBY, AT91_PM_ULP0, AT91_PM_ULP1, AT91_PM_BACKUP,
>>>>> };
>>>>> static const u32 iomaps[] __initconst = {
>>>>> - [AT91_PM_ULP0] = AT91_PM_IOMAP(SFRBU),
>>>>> + [AT91_PM_ULP0] = AT91_PM_IOMAP(SFRBU) |
>>>>> + AT91_PM_IOMAP(SHDWC),
>>>>
>>>> In theory, as the wakeup sources can also resumes the system from standby
>>>> (WFI), the shdwc should be mapped for standby, too. Unless I'm wrong and
>>>> the wakeup sources covered by the SHDWC_SR register don't apply to standby
>>>> (WFI).
>>> The device can wake up from an RTT or RTC alarm event on both the
>>> standby power mode and the ULP0 power mode, since the RTT/RTC are
>>> included in the SHDWC_SR I think it is safe to have this.
>>> If I understand what you are asking correctly.
>>
>> I was asking if the SHDWC should also be mapped for standby like:
> Ok I see. I have a better understanding now of wake up sources table
> like you showed below. I think for readability of code I should not have
> SHDWC set as ULP0 and STANDBY source because in at91_pm_config_ws()
> SHDWC is only configured as a wake up source in ULP1 power mode.
>
> So removing SHDWC from the ULP0 wake up source would reflect more
> accurately what is configured as a wake up source in the code. What do
> you think?
Sounds good.
Thank you,
Claudiu
next prev parent reply other threads:[~2025-02-24 8:55 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-02-10 21:13 [PATCH v2 00/15] Enable Power Modes Support for SAMA7D65 SoC Ryan.Wanner
2025-02-10 21:13 ` [PATCH v2 01/15] dt-bindings: mfd: syscon: add microchip,sama7d65-ddr3phy Ryan.Wanner
2025-02-11 8:12 ` Krzysztof Kozlowski
2025-02-11 8:12 ` Krzysztof Kozlowski
2025-02-10 21:13 ` [PATCH v2 02/15] dt-bindings: mfd: syscon: add microchip,sama7d65-sfrbu Ryan.Wanner
2025-02-11 8:14 ` Krzysztof Kozlowski
2025-02-11 8:14 ` Krzysztof Kozlowski
2025-02-13 20:30 ` Conor Dooley
2025-02-13 20:30 ` Conor Dooley
2025-02-14 7:20 ` Krzysztof Kozlowski
2025-02-14 7:20 ` Krzysztof Kozlowski
2025-02-10 21:13 ` [PATCH v2 03/15] dt-bindings: sram: Add microchip,sama7d65-sram Ryan.Wanner
2025-02-10 21:13 ` [PATCH v2 04/15] dt-bindings: power: reset: atmel,sama5d2-shdwc: Add microchip,sama7d65-shdwc Ryan.Wanner
2025-02-10 21:13 ` [PATCH v2 05/15] dt-bindings: reset: atmel,at91sam9260-reset: add microchip,sama7d65-rstc Ryan.Wanner
2025-02-14 8:27 ` Krzysztof Kozlowski
2025-02-14 8:27 ` Krzysztof Kozlowski
2025-02-10 21:13 ` [PATCH v2 06/15] dt-bindings: rtc: at91rm9200: add microchip,sama7d65-rtc Ryan.Wanner
2025-02-12 8:18 ` Claudiu Beznea
2025-02-14 8:27 ` Krzysztof Kozlowski
2025-02-14 8:27 ` Krzysztof Kozlowski
2025-02-10 21:13 ` [PATCH v2 07/15] dt-bindings: at91rm9260-rtt: add microchip,sama7d65-rtt Ryan.Wanner
2025-02-12 8:18 ` Claudiu Beznea
2025-02-14 8:28 ` Krzysztof Kozlowski
2025-02-14 8:28 ` Krzysztof Kozlowski
2025-02-10 21:13 ` [PATCH v2 08/15] ARM: at91: Add PM support to sama7d65 Ryan.Wanner
2025-02-10 21:13 ` [PATCH v2 09/15] ARM: at91: pm: fix at91_suspend_finish for ZQ calibration Ryan.Wanner
2025-02-12 8:17 ` Claudiu Beznea
2025-02-10 21:13 ` [PATCH v2 10/15] ARM: at91: pm: add DT compatible support for sama7d65 Ryan.Wanner
2025-02-12 8:20 ` Claudiu Beznea
2025-02-10 21:13 ` [PATCH v2 11/15] ARM: at91: PM: Add Backup mode for SAMA7D65 Ryan.Wanner
2025-02-12 8:15 ` Claudiu Beznea
2025-02-10 21:13 ` [PATCH v2 12/15] ARM: at91: pm: Enable ULP0 " Ryan.Wanner
2025-02-13 8:20 ` Claudiu Beznea
2025-02-14 18:09 ` Ryan.Wanner
2025-02-17 7:18 ` Claudiu Beznea
2025-02-19 15:24 ` Ryan.Wanner
2025-02-24 8:55 ` Claudiu Beznea [this message]
2025-02-10 21:13 ` [PATCH v2 13/15] power: reset: at91-sama5d2_shdwc: Add sama7d65 PMC Ryan.Wanner
2025-02-12 8:20 ` Claudiu Beznea
2025-02-10 21:13 ` [PATCH v2 14/15] ARM: dts: microchip: sama7d65: Add Reset and Shutdown and PM support Ryan.Wanner
2025-02-13 8:28 ` Claudiu Beznea
2025-02-10 21:13 ` [PATCH v2 15/15] ARM: dts: microchip: add shutdown controller and rtt timer Ryan.Wanner
2025-02-13 8:30 ` Claudiu Beznea
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=edb77ae7-d837-4e30-99ec-9ecb8b0647e9@tuxon.dev \
--to=claudiu.beznea@tuxon.dev \
--cc=Nicolas.Ferre@microchip.com \
--cc=Ryan.Wanner@microchip.com \
--cc=alexandre.belloni@bootlin.com \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=krzk+dt@kernel.org \
--cc=lee@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=linux-rtc@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=p.zabel@pengutronix.de \
--cc=robh@kernel.org \
--cc=sre@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.