From: Romain Izard <romain.izard.pro@gmail.com>
To: Nicolas Ferre <nicolas.ferre@microchip.com>
Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com>,
Boris Brezillon <boris.brezillon@free-electrons.com>,
Michael Turquette <mturquette@baylibre.com>,
Stephen Boyd <sboyd@codeaurora.org>,
Ludovic Desroches <ludovic.desroches@microchip.com>,
Wenyou Yang <wenyou.yang@atmel.com>,
Josh Wu <rainyfeeling@outlook.com>,
David Woodhouse <dwmw2@infradead.org>,
Brian Norris <computersforpeace@gmail.com>,
Marek Vasut <marek.vasut@gmail.com>,
Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>,
Thierry Reding <thierry.reding@gmail.com>,
Richard Genoud <richard.genoud@gmail.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Alan Stern <stern@rowland.harvard.edu>,
linux-clk@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
linux-mtd <linux-mtd@lists.infradead.org>,
linux-pwm@vger.kernel.org, linux-serial@vger.kernel.org,
linux-usb@vger.kernel.org,
linux-arm-kernel <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v2 3/9] clk: at91: pmc: Support backup for programmable clocks
Date: Mon, 25 Sep 2017 10:25:35 +0200 [thread overview]
Message-ID: <CAGkQfmOQ_5hb-OKWQxjboPS1V0v-jCPxyyDZyCyZLFYFoeXWbA@mail.gmail.com> (raw)
In-Reply-To: <d6359e89-7c78-fdec-fbab-eb885017ec58@microchip.com>
2017-09-22 12:31 GMT+02:00 Nicolas Ferre <nicolas.ferre@microchip.com>:
> On 15/09/2017 at 16:04, Romain Izard wrote:
>> From: Romain Izard <romain.izard@mobile-devices.fr>
>>
>> When an AT91 programmable clock is declared in the device tree, register
>> it into the Power Management Controller driver. On entering suspend mode,
>> the driver saves and restores the Programmable Clock registers to support
>> the backup mode for these clocks.
>>
>> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
>
> Romain,
>
> Some nitpicking and one comment. But on the overall patch, here is my:
> Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
>
> See below:
>
>> ---
>> Changes in v2:
>> * register PCKs on clock startup
>>
>> drivers/clk/at91/clk-programmable.c | 2 ++
>> drivers/clk/at91/pmc.c | 27 +++++++++++++++++++++++++++
>> drivers/clk/at91/pmc.h | 2 ++
>> 3 files changed, 31 insertions(+)
>>
>> diff --git a/drivers/clk/at91/clk-programmable.c b/drivers/clk/at91/clk-programmable.c
>> index 85a449cf61e3..0e6aab1252fc 100644
>> --- a/drivers/clk/at91/clk-programmable.c
>> +++ b/drivers/clk/at91/clk-programmable.c
>> @@ -204,6 +204,8 @@ at91_clk_register_programmable(struct regmap *regmap,
>> if (ret) {
>> kfree(prog);
>> hw = ERR_PTR(ret);
>
> Nit: "else" not needed.
>
This is a shared idiom in all the atmel clock drivers, so I prefer to keep
it this way.
>> + } else {
>> + pmc_register_pck(id);
>> }
>>
>> return hw;
>> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
>> index 07dc2861ad3f..3910b7537152 100644
>> --- a/drivers/clk/at91/pmc.c
>> +++ b/drivers/clk/at91/pmc.c
>> @@ -22,6 +22,7 @@
>> #include "pmc.h"
>>
>> #define PMC_MAX_IDS 128
>> +#define PMC_MAX_PCKS 8
>>
>> int of_at91_get_clk_range(struct device_node *np, const char *propname,
>> struct clk_range *range)
>> @@ -50,6 +51,7 @@ EXPORT_SYMBOL_GPL(of_at91_get_clk_range);
>> static struct regmap *pmcreg;
>>
>> static u8 registered_ids[PMC_MAX_IDS];
>> +static u8 registered_pcks[PMC_MAX_PCKS];
>>
>> static struct
>> {
>> @@ -66,8 +68,10 @@ static struct
>> u32 pcr[PMC_MAX_IDS];
>> u32 audio_pll0;
>> u32 audio_pll1;
>> + u32 pckr[PMC_MAX_PCKS];
>> } pmc_cache;
>>
>> +/* Clock ID 0 is invalid */
>
> (read: so we can use the 0 value as an indicator that this place in the
> table hasn't been filled, so unused)
>
>> void pmc_register_id(u8 id)
>> {
>> int i;
>> @@ -82,6 +86,21 @@ void pmc_register_id(u8 id)
>> }
>> }
>>
>> +/* Programmable Clock 0 is valid */
>
> I understand the rationale behind these ^^ two comments, but I would
> like that it's more explicit. Saying that you will store the pck id as
> (id + 1) and that you would have to invert this operation while using
> the stored id.
> Maybe add a comment about this transformation to the struct definition
> as well...
>
I will improve the comments for the next revision.
>
>> +void pmc_register_pck(u8 pck)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < PMC_MAX_PCKS; i++) {
>> + if (registered_pcks[i] == 0) {
>> + registered_pcks[i] = pck + 1;
>> + break;
>> + }
>> + if (registered_pcks[i] == (pck + 1))
>> + break;
>> + }
>> +}
>> +
>> static int pmc_suspend(void)
>> {
>> int i;
>> @@ -103,6 +122,10 @@ static int pmc_suspend(void)
>> regmap_read(pmcreg, AT91_PMC_PCR,
>> &pmc_cache.pcr[registered_ids[i]]);
>> }
>> + for (i = 0; registered_pcks[i]; i++) {
>> + u8 num = registered_pcks[i] - 1;
>
> Nit: declaration are better made at the beginning of the function. This
> lead to a checkpatch warning:
> "WARNING: Missing a blank line after declarations"
>
I'll fix this as well.
>> + regmap_read(pmcreg, AT91_PMC_PCKR(num), &pmc_cache.pckr[num]);
>> + }
>>
>> return 0;
>> }
>> @@ -143,6 +166,10 @@ static void pmc_resume(void)
>> pmc_cache.pcr[registered_ids[i]] |
>> AT91_PMC_PCR_CMD);
>> }
>> + for (i = 0; registered_pcks[i]; i++) {
>> + u8 num = registered_pcks[i] - 1;
>
> Ditto
>
>> + regmap_write(pmcreg, AT91_PMC_PCKR(num), pmc_cache.pckr[num]);
>> + }
>>
>> if (pmc_cache.uckr & AT91_PMC_UPLLEN)
>> mask |= AT91_PMC_LOCKU;
>> diff --git a/drivers/clk/at91/pmc.h b/drivers/clk/at91/pmc.h
>> index 858e8ef7e8db..d22b1fa9ecdc 100644
>> --- a/drivers/clk/at91/pmc.h
>> +++ b/drivers/clk/at91/pmc.h
>> @@ -31,8 +31,10 @@ int of_at91_get_clk_range(struct device_node *np, const char *propname,
>>
>> #ifdef CONFIG_PM
>> void pmc_register_id(u8 id);
>> +void pmc_register_pck(u8 pck);
>> #else
>> static inline void pmc_register_id(u8 id) {}
>> +static inline void pmc_register_pck(u8 pck) {}
>> #endif
>>
>> #endif /* __PMC_H_ */
>>
--
Romain Izard
next prev parent reply other threads:[~2017-09-25 8:26 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-15 14:04 [PATCH v2 0/9] Various patches for SAMA5D2 backup mode Romain Izard
2017-09-15 14:04 ` [PATCH v2 1/9] clk: at91: pmc: Wait for clocks when resuming Romain Izard
2017-09-22 12:05 ` Ludovic Desroches
2017-09-22 12:13 ` Nicolas Ferre
2017-09-15 14:04 ` [PATCH v2 2/9] clk: at91: pmc: Save SCSR during suspend Romain Izard
2017-09-15 14:04 ` [PATCH v2 3/9] clk: at91: pmc: Support backup for programmable clocks Romain Izard
2017-09-22 10:31 ` Nicolas Ferre
2017-09-25 8:25 ` Romain Izard [this message]
2017-09-15 14:04 ` [PATCH v2 4/9] mtd: nand: atmel: Avoid ECC errors when leaving backup mode Romain Izard
2017-09-18 9:50 ` Boris Brezillon
2017-09-15 14:04 ` [PATCH v2 5/9] mtd: nand: atmel: Report PMECC failures as errors Romain Izard
2017-09-18 10:00 ` Boris Brezillon
2017-09-21 9:22 ` Romain Izard
2017-09-15 14:04 ` [PATCH v2 6/9] ehci-atmel: Power down during suspend is normal Romain Izard
2017-09-22 12:40 ` Nicolas Ferre
2017-09-15 14:04 ` [PATCH v2 7/9] pwm: atmel-tcb: Support backup mode Romain Izard
2017-09-22 12:42 ` Nicolas Ferre
2017-09-15 14:04 ` [PATCH v2 8/9] atmel_flexcom: " Romain Izard
2017-09-19 9:29 ` Nicolas Ferre
2017-09-19 15:25 ` Lee Jones
2017-09-20 8:30 ` Romain Izard
2017-09-20 9:18 ` Alexandre Belloni
2017-09-15 14:04 ` [PATCH v2 9/9] tty/serial: atmel: Prevent a warning on suspend Romain Izard
2017-09-19 10:19 ` Nicolas Ferre
2017-09-20 14:35 ` Richard Genoud
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=CAGkQfmOQ_5hb-OKWQxjboPS1V0v-jCPxyyDZyCyZLFYFoeXWbA@mail.gmail.com \
--to=romain.izard.pro@gmail.com \
--cc=alexandre.belloni@free-electrons.com \
--cc=boris.brezillon@free-electrons.com \
--cc=computersforpeace@gmail.com \
--cc=cyrille.pitchen@wedev4u.fr \
--cc=dwmw2@infradead.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=linux-pwm@vger.kernel.org \
--cc=linux-serial@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=ludovic.desroches@microchip.com \
--cc=marek.vasut@gmail.com \
--cc=mturquette@baylibre.com \
--cc=nicolas.ferre@microchip.com \
--cc=rainyfeeling@outlook.com \
--cc=richard.genoud@gmail.com \
--cc=sboyd@codeaurora.org \
--cc=stern@rowland.harvard.edu \
--cc=thierry.reding@gmail.com \
--cc=wenyou.yang@atmel.com \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).