[{"id":1773454,"web_url":"http://patchwork.ozlabs.org/comment/1773454/","msgid":"","list_archive_url":null,"date":"2017-09-22T10:31:12","subject":"Re: [PATCH v2 3/9] clk: at91: pmc: Support backup for programmable\n\tclocks","submitter":{"id":71036,"url":"http://patchwork.ozlabs.org/api/people/71036/","name":"Nicolas Ferre","email":"nicolas.ferre@microchip.com"},"content":"On 15/09/2017 at 16:04, Romain Izard wrote:\n> From: Romain Izard \n> \n> When an AT91 programmable clock is declared in the device tree, register\n> it into the Power Management Controller driver. On entering suspend mode,\n> the driver saves and restores the Programmable Clock registers to support\n> the backup mode for these clocks.\n> \n> Signed-off-by: Romain Izard \n\nRomain,\n\nSome nitpicking and one comment. But on the overall patch, here is my:\nAcked-by: Nicolas Ferre \n\nSee below:\n\n> ---\n> Changes in v2:\n> * register PCKs on clock startup\n> \n> drivers/clk/at91/clk-programmable.c | 2 ++\n> drivers/clk/at91/pmc.c | 27 +++++++++++++++++++++++++++\n> drivers/clk/at91/pmc.h | 2 ++\n> 3 files changed, 31 insertions(+)\n> \n> diff --git a/drivers/clk/at91/clk-programmable.c b/drivers/clk/at91/clk-programmable.c\n> index 85a449cf61e3..0e6aab1252fc 100644\n> --- a/drivers/clk/at91/clk-programmable.c\n> +++ b/drivers/clk/at91/clk-programmable.c\n> @@ -204,6 +204,8 @@ at91_clk_register_programmable(struct regmap *regmap,\n> \tif (ret) {\n> \t\tkfree(prog);\n> \t\thw = ERR_PTR(ret);\n\nNit: \"else\" not needed.\n\n> +\t} else {\n> +\t\tpmc_register_pck(id);\n> \t}\n> \n> \treturn hw;\n> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c\n> index 07dc2861ad3f..3910b7537152 100644\n> --- a/drivers/clk/at91/pmc.c\n> +++ b/drivers/clk/at91/pmc.c\n> @@ -22,6 +22,7 @@\n> #include \"pmc.h\"\n> \n> #define PMC_MAX_IDS 128\n> +#define PMC_MAX_PCKS 8\n> \n> int of_at91_get_clk_range(struct device_node *np, const char *propname,\n> \t\t\t struct clk_range *range)\n> @@ -50,6 +51,7 @@ EXPORT_SYMBOL_GPL(of_at91_get_clk_range);\n> static struct regmap *pmcreg;\n> \n> static u8 registered_ids[PMC_MAX_IDS];\n> +static u8 registered_pcks[PMC_MAX_PCKS];\n> \n> static struct\n> {\n> @@ -66,8 +68,10 @@ static struct\n> \tu32 pcr[PMC_MAX_IDS];\n> \tu32 audio_pll0;\n> \tu32 audio_pll1;\n> +\tu32 pckr[PMC_MAX_PCKS];\n> } pmc_cache;\n> \n> +/* Clock ID 0 is invalid */\n\n(read: so we can use the 0 value as an indicator that this place in the\ntable hasn't been filled, so unused)\n\n> void pmc_register_id(u8 id)\n> {\n> \tint i;\n> @@ -82,6 +86,21 @@ void pmc_register_id(u8 id)\n> \t}\n> }\n> \n> +/* Programmable Clock 0 is valid */\n\nI understand the rationale behind these ^^ two comments, but I would\nlike that it's more explicit. Saying that you will store the pck id as\n(id + 1) and that you would have to invert this operation while using\nthe stored id.\nMaybe add a comment about this transformation to the struct definition\nas well...\n\n\n> +void pmc_register_pck(u8 pck)\n> +{\n> +\tint i;\n> +\n> +\tfor (i = 0; i < PMC_MAX_PCKS; i++) {\n> +\t\tif (registered_pcks[i] == 0) {\n> +\t\t\tregistered_pcks[i] = pck + 1;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tif (registered_pcks[i] == (pck + 1))\n> +\t\t\tbreak;\n> +\t}\n> +}\n> +\n> static int pmc_suspend(void)\n> {\n> \tint i;\n> @@ -103,6 +122,10 @@ static int pmc_suspend(void)\n> \t\tregmap_read(pmcreg, AT91_PMC_PCR,\n> \t\t\t &pmc_cache.pcr[registered_ids[i]]);\n> \t}\n> +\tfor (i = 0; registered_pcks[i]; i++) {\n> +\t\tu8 num = registered_pcks[i] - 1;\n\nNit: declaration are better made at the beginning of the function. This\nlead to a checkpatch warning:\n\"WARNING: Missing a blank line after declarations\"\n\n> +\t\tregmap_read(pmcreg, AT91_PMC_PCKR(num), &pmc_cache.pckr[num]);\n> +\t}\n> \n> \treturn 0;\n> }\n> @@ -143,6 +166,10 @@ static void pmc_resume(void)\n> \t\t\t pmc_cache.pcr[registered_ids[i]] |\n> \t\t\t AT91_PMC_PCR_CMD);\n> \t}\n> +\tfor (i = 0; registered_pcks[i]; i++) {\n> +\t\tu8 num = registered_pcks[i] - 1;\n\nDitto\n\n> +\t\tregmap_write(pmcreg, AT91_PMC_PCKR(num), pmc_cache.pckr[num]);\n> +\t}\n> \n> \tif (pmc_cache.uckr & AT91_PMC_UPLLEN)\n> \t\tmask |= AT91_PMC_LOCKU;\n> diff --git a/drivers/clk/at91/pmc.h b/drivers/clk/at91/pmc.h\n> index 858e8ef7e8db..d22b1fa9ecdc 100644\n> --- a/drivers/clk/at91/pmc.h\n> +++ b/drivers/clk/at91/pmc.h\n> @@ -31,8 +31,10 @@ int of_at91_get_clk_range(struct device_node *np, const char *propname,\n> \n> #ifdef CONFIG_PM\n> void pmc_register_id(u8 id);\n> +void pmc_register_pck(u8 pck);\n> #else\n> static inline void pmc_register_id(u8 id) {}\n> +static inline void pmc_register_pck(u8 pck) {}\n> #endif\n> \n> #endif /* __PMC_H_ */\n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-pwm-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xz8r271h9z9s7h\n\tfor ;\n\tFri, 22 Sep 2017 20:30:10 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751981AbdIVKaJ (ORCPT );\n\tFri, 22 Sep 2017 06:30:09 -0400","from esa3.microchip.iphmx.com ([68.232.153.233]:17130 \"EHLO\n\tesa3.microchip.iphmx.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751872AbdIVKaI (ORCPT\n\t); Fri, 22 Sep 2017 06:30:08 -0400","from smtpout.microchip.com (HELO email.microchip.com)\n\t([198.175.253.82])\n\tby esa3.microchip.iphmx.com with ESMTP/TLS/DHE-RSA-AES256-SHA;\n\t22 Sep 2017 03:30:06 -0700","from [10.159.245.112] (10.10.76.4) by chn-sv-exch05.mchp-main.com\n\t(10.10.76.106) with Microsoft SMTP Server id 14.3.352.0;\n\tFri, 22 Sep 2017 03:30:05 -0700"],"X-IronPort-AV":"E=Sophos;i=\"5.42,427,1500966000\"; d=\"scan'208\";a=\"7270387\"","Subject":"Re: [PATCH v2 3/9] clk: at91: pmc: Support backup for programmable\n\tclocks","To":"Romain Izard ,\n\tAlexandre Belloni ,\n\tBoris Brezillon ,\n\tMichael Turquette ,\n\tStephen Boyd ,\n\tLudovic Desroches ,\n\tWenyou Yang , Josh Wu ,\n\tDavid Woodhouse ,\n\tBrian Norris ,\n\tMarek Vasut ,\n\tCyrille Pitchen ,\n\tThierry Reding ,\n\tRichard Genoud ,\n\tGreg Kroah-Hartman ,\n\tAlan Stern ","CC":", ,\n\t, ,\n\t, ,\n\t,\n\tRomain Izard ","References":"<20170915140411.31716-1-romain.izard.pro@gmail.com>\n\t<20170915140411.31716-4-romain.izard.pro@gmail.com>","From":"Nicolas Ferre ","Organization":"microchip","Message-ID":"","Date":"Fri, 22 Sep 2017 12:31:12 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170915140411.31716-4-romain.izard.pro@gmail.com>","Content-Type":"text/plain; charset=\"utf-8\"","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Sender":"linux-pwm-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-pwm@vger.kernel.org"}},{"id":1774507,"web_url":"http://patchwork.ozlabs.org/comment/1774507/","msgid":"","list_archive_url":null,"date":"2017-09-25T08:25:35","subject":"Re: [PATCH v2 3/9] clk: at91: pmc: Support backup for programmable\n\tclocks","submitter":{"id":8236,"url":"http://patchwork.ozlabs.org/api/people/8236/","name":"Romain Izard","email":"romain.izard.pro@gmail.com"},"content":"2017-09-22 12:31 GMT+02:00 Nicolas Ferre :\n> On 15/09/2017 at 16:04, Romain Izard wrote:\n>> From: Romain Izard \n>>\n>> When an AT91 programmable clock is declared in the device tree, register\n>> it into the Power Management Controller driver. On entering suspend mode,\n>> the driver saves and restores the Programmable Clock registers to support\n>> the backup mode for these clocks.\n>>\n>> Signed-off-by: Romain Izard \n>\n> Romain,\n>\n> Some nitpicking and one comment. But on the overall patch, here is my:\n> Acked-by: Nicolas Ferre \n>\n> See below:\n>\n>> ---\n>> Changes in v2:\n>> * register PCKs on clock startup\n>>\n>> drivers/clk/at91/clk-programmable.c | 2 ++\n>> drivers/clk/at91/pmc.c | 27 +++++++++++++++++++++++++++\n>> drivers/clk/at91/pmc.h | 2 ++\n>> 3 files changed, 31 insertions(+)\n>>\n>> diff --git a/drivers/clk/at91/clk-programmable.c b/drivers/clk/at91/clk-programmable.c\n>> index 85a449cf61e3..0e6aab1252fc 100644\n>> --- a/drivers/clk/at91/clk-programmable.c\n>> +++ b/drivers/clk/at91/clk-programmable.c\n>> @@ -204,6 +204,8 @@ at91_clk_register_programmable(struct regmap *regmap,\n>> if (ret) {\n>> kfree(prog);\n>> hw = ERR_PTR(ret);\n>\n> Nit: \"else\" not needed.\n>\nThis is a shared idiom in all the atmel clock drivers, so I prefer to keep\nit this way.\n\n>> + } else {\n>> + pmc_register_pck(id);\n>> }\n>>\n>> return hw;\n>> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c\n>> index 07dc2861ad3f..3910b7537152 100644\n>> --- a/drivers/clk/at91/pmc.c\n>> +++ b/drivers/clk/at91/pmc.c\n>> @@ -22,6 +22,7 @@\n>> #include \"pmc.h\"\n>>\n>> #define PMC_MAX_IDS 128\n>> +#define PMC_MAX_PCKS 8\n>>\n>> int of_at91_get_clk_range(struct device_node *np, const char *propname,\n>> struct clk_range *range)\n>> @@ -50,6 +51,7 @@ EXPORT_SYMBOL_GPL(of_at91_get_clk_range);\n>> static struct regmap *pmcreg;\n>>\n>> static u8 registered_ids[PMC_MAX_IDS];\n>> +static u8 registered_pcks[PMC_MAX_PCKS];\n>>\n>> static struct\n>> {\n>> @@ -66,8 +68,10 @@ static struct\n>> u32 pcr[PMC_MAX_IDS];\n>> u32 audio_pll0;\n>> u32 audio_pll1;\n>> + u32 pckr[PMC_MAX_PCKS];\n>> } pmc_cache;\n>>\n>> +/* Clock ID 0 is invalid */\n>\n> (read: so we can use the 0 value as an indicator that this place in the\n> table hasn't been filled, so unused)\n>\n>> void pmc_register_id(u8 id)\n>> {\n>> int i;\n>> @@ -82,6 +86,21 @@ void pmc_register_id(u8 id)\n>> }\n>> }\n>>\n>> +/* Programmable Clock 0 is valid */\n>\n> I understand the rationale behind these ^^ two comments, but I would\n> like that it's more explicit. Saying that you will store the pck id as\n> (id + 1) and that you would have to invert this operation while using\n> the stored id.\n> Maybe add a comment about this transformation to the struct definition\n> as well...\n>\n\nI will improve the comments for the next revision.\n\n>\n>> +void pmc_register_pck(u8 pck)\n>> +{\n>> + int i;\n>> +\n>> + for (i = 0; i < PMC_MAX_PCKS; i++) {\n>> + if (registered_pcks[i] == 0) {\n>> + registered_pcks[i] = pck + 1;\n>> + break;\n>> + }\n>> + if (registered_pcks[i] == (pck + 1))\n>> + break;\n>> + }\n>> +}\n>> +\n>> static int pmc_suspend(void)\n>> {\n>> int i;\n>> @@ -103,6 +122,10 @@ static int pmc_suspend(void)\n>> regmap_read(pmcreg, AT91_PMC_PCR,\n>> &pmc_cache.pcr[registered_ids[i]]);\n>> }\n>> + for (i = 0; registered_pcks[i]; i++) {\n>> + u8 num = registered_pcks[i] - 1;\n>\n> Nit: declaration are better made at the beginning of the function. This\n> lead to a checkpatch warning:\n> \"WARNING: Missing a blank line after declarations\"\n>\n\nI'll fix this as well.\n\n>> + regmap_read(pmcreg, AT91_PMC_PCKR(num), &pmc_cache.pckr[num]);\n>> + }\n>>\n>> return 0;\n>> }\n>> @@ -143,6 +166,10 @@ static void pmc_resume(void)\n>> pmc_cache.pcr[registered_ids[i]] |\n>> AT91_PMC_PCR_CMD);\n>> }\n>> + for (i = 0; registered_pcks[i]; i++) {\n>> + u8 num = registered_pcks[i] - 1;\n>\n> Ditto\n>\n>> + regmap_write(pmcreg, AT91_PMC_PCKR(num), pmc_cache.pckr[num]);\n>> + }\n>>\n>> if (pmc_cache.uckr & AT91_PMC_UPLLEN)\n>> mask |= AT91_PMC_LOCKU;\n>> diff --git a/drivers/clk/at91/pmc.h b/drivers/clk/at91/pmc.h\n>> index 858e8ef7e8db..d22b1fa9ecdc 100644\n>> --- a/drivers/clk/at91/pmc.h\n>> +++ b/drivers/clk/at91/pmc.h\n>> @@ -31,8 +31,10 @@ int of_at91_get_clk_range(struct device_node *np, const char *propname,\n>>\n>> #ifdef CONFIG_PM\n>> void pmc_register_id(u8 id);\n>> +void pmc_register_pck(u8 pck);\n>> #else\n>> static inline void pmc_register_id(u8 id) {}\n>> +static inline void pmc_register_pck(u8 pck) {}\n>> #endif\n>>\n>> #endif /* __PMC_H_ */\n>>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-pwm-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tsecure) header.d=mobile-devices.fr header.i=@mobile-devices.fr\n\theader.b=\"u9FIJbWo\"; \n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"QC1fLgzK\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y0xxV58C6z9tXH\n\tfor ;\n\tMon, 25 Sep 2017 18:26:06 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S934213AbdIYI0C (ORCPT );\n\tMon, 25 Sep 2017 04:26:02 -0400","from mail-qk0-f175.google.com ([209.85.220.175]:54558 \"EHLO\n\tmail-qk0-f175.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S934199AbdIYIZ4 (ORCPT\n\t); Mon, 25 Sep 2017 04:25:56 -0400","by mail-qk0-f175.google.com with SMTP id d70so5892151qkc.11\n\tfor ; Mon, 25 Sep 2017 01:25:56 -0700 (PDT)","by 10.200.48.120 with HTTP; Mon, 25 Sep 2017 01:25:35 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=mobile-devices.fr; s=google;\n\th=mime-version:sender:in-reply-to:references:from:date:message-id\n\t:subject:to:cc;\n\tbh=2e8+oWffspBbHMJrd9Dff9/rMtoYbYBla7l3jBGVGs0=;\n\tb=u9FIJbWoruJHGktylIkIpDwdKhN/aXzBv7CrxO4cDUWJWDvpnt8FCbhK1dp/rzkRTO\n\tLGWUUC2v2rbxTYYSAF0qVGKUNWaPGhqlID9qVUacJ2JRT4GlhEj18ctqiOWWczGIdLg9\n\tBWr0tsQghPxCsG1bO8zcz6H9ObrEiMPzSBIp8=","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20161025;\n\th=mime-version:sender:in-reply-to:references:from:date:message-id\n\t:subject:to:cc;\n\tbh=2e8+oWffspBbHMJrd9Dff9/rMtoYbYBla7l3jBGVGs0=;\n\tb=QC1fLgzKqfDIrdZOZb5WyX71qT/I0SJJOoUzF4NKHALgk/D/qX1Cs8r7qFAa9YOJm+\n\tTHgaIUVjb16Ov/eY50tJH3C/WDOun2Vrxy6qbZ8xJ4CDbdpTYbzmPcL6la0eO0To6yGa\n\tx52X2VoOwXKIeYnpZgRH+e/6yAtCbvlvzeGJuqr44nzf6JMWREfz4/HtU4yfFEKtQGP3\n\t/XhphCBQ09JWL3SJQ9xCXZK39hZxHiDZC6CNAt5NyNwhgZkLphxXmaWeqGgrV/yOFUGC\n\tYXTIJMpj7Souoe1aOkTCrNWtHQ1hy/c1YSzRyoTTt+Q3DnQvxvgHjQaruXUMgLu9USOR\n\t/ehA=="],"X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:sender:in-reply-to:references:from\n\t:date:message-id:subject:to:cc;\n\tbh=2e8+oWffspBbHMJrd9Dff9/rMtoYbYBla7l3jBGVGs0=;\n\tb=RNuhVKoPjH1/me3GrHW7BRnBTxdU6cjctHD1D2DEhnCTQ0/c9R+Gz/w1CUMqEy8aJv\n\tub2vNc9/oT+8lNFIKZAgCPhThmGxA/yhPlg4ync8hOFUIPMYKBDEg/Mpmx5eKfejYfl7\n\t9NciJ08gkyuZY44cau7EtWFXhiCHx/bORo3LM7wSXeyIbGKZDBTeshcX7Zwyf7iq2QFS\n\t0zNMTo2NnupmdBqk8ErICFdBgs4maDMQLAeXxmw5Z/nDrCgOdFW3APzpTAaEMxriCh+e\n\tLPoqnkATqLCnRaQJNui6pE2WAITsQx/+VSuqbgIrGAX2pLeRiwxJLcgzagfsV2WIbfK6\n\twPyA==","X-Gm-Message-State":"AHPjjUgpfN72oRPIkEGscLnX3Wz4j1aIJ21kUYBCwKx2DdiHgiQ9qS7e\n\tSEun+e3j1R6kHZuAPOdQaHGE3luZg3aaggzlMz86ag==","X-Google-Smtp-Source":"AOwi7QAZ353KisEOLiiZWWAjRLtDomceB0e6zs/k3T35vmLl+CPfjZx7Mswq3236QjVU6WpBkvnx7IU1EpxjaixGchQ=","X-Received":"by 10.55.154.4 with SMTP id c4mr9270814qke.131.1506327955703;\n\tMon, 25 Sep 2017 01:25:55 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"","References":"<20170915140411.31716-1-romain.izard.pro@gmail.com>\n\t<20170915140411.31716-4-romain.izard.pro@gmail.com>\n\t","From":"Romain Izard ","Date":"Mon, 25 Sep 2017 10:25:35 +0200","X-Google-Sender-Auth":"fyBRz7UOQm_YHyxzQiPq1I9ZrB0","Message-ID":"","Subject":"Re: [PATCH v2 3/9] clk: at91: pmc: Support backup for programmable\n\tclocks","To":"Nicolas Ferre ","Cc":"Alexandre Belloni ,\n\tBoris Brezillon ,\n\tMichael Turquette ,\n\tStephen Boyd ,\n\tLudovic Desroches ,\n\tWenyou Yang , Josh Wu ,\n\tDavid Woodhouse ,\n\tBrian Norris ,\n\tMarek Vasut ,\n\tCyrille Pitchen ,\n\tThierry Reding ,\n\tRichard Genoud ,\n\tGreg Kroah-Hartman ,\n\tAlan Stern ,\n\tlinux-clk@vger.kernel.org, LKML ,\n\tlinux-mtd ,\n\tlinux-pwm@vger.kernel.org, linux-serial@vger.kernel.org,\n\tlinux-usb@vger.kernel.org,\n\tlinux-arm-kernel ","Content-Type":"text/plain; charset=\"UTF-8\"","Sender":"linux-pwm-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-pwm@vger.kernel.org"}}]