From: Sam Protsenko <semen.protsenko@linaro.org> To: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> Cc: "Sylwester Nawrocki" <s.nawrocki@samsung.com>, "Paweł Chmiel" <pawel.mikolaj.chmiel@gmail.com>, "Chanwoo Choi" <cw00.choi@samsung.com>, "Tomasz Figa" <tomasz.figa@gmail.com>, "Rob Herring" <robh+dt@kernel.org>, "Stephen Boyd" <sboyd@kernel.org>, "Michael Turquette" <mturquette@baylibre.com>, "Ryu Euiyoul" <ryu.real@samsung.com>, "Tom Gall" <tom.gall@linaro.org>, "Sumit Semwal" <sumit.semwal@linaro.org>, "John Stultz" <john.stultz@linaro.org>, "Amit Pundir" <amit.pundir@linaro.org>, devicetree <devicetree@vger.kernel.org>, "linux-arm Mailing List" <linux-arm-kernel@lists.infradead.org>, linux-clk <linux-clk@vger.kernel.org>, "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>, "Linux Samsung SOC" <linux-samsung-soc@vger.kernel.org> Subject: Re: [PATCH 1/6] clk: samsung: Enable bus clock on init Date: Wed, 6 Oct 2021 16:29:53 +0300 [thread overview] Message-ID: <CAPLW+4kq7MuF8HiY-UYvC0b8woT6G=hktJPPLD0iQ39iddmceA@mail.gmail.com> (raw) In-Reply-To: <16ee07a1-afa9-b258-8836-e96de84551db@canonical.com> On Wed, 6 Oct 2021 at 15:38, Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > On 06/10/2021 12:46, Sam Protsenko wrote: > > On Wed, 15 Sept 2021 at 11:21, Krzysztof Kozlowski > > <krzysztof.kozlowski@canonical.com> wrote: > >> > >> On 14/09/2021 17:56, Sam Protsenko wrote: > >>> By default if bus clock has no users its "enable count" value is 0. It > >>> might be actually running if it's already enabled in bootloader, but > >>> then in some cases it can be disabled by mistake. For example, such case > >>> was observed when dw_mci_probe() enabled bus clock, then failed to do > >>> something and disabled that bus clock on error path. After that even > >>> attempt to read the 'clk_summary' file in DebugFS freezed forever, as > >>> CMU bus clock ended up being disabled and it wasn't possible to access > >>> CMU registers anymore. > >>> > >>> To avoid such cases, CMU driver must increment the ref count for that > >>> bus clock by running clk_prepare_enable(). There is already existing > >>> '.clk_name' field in struct samsung_cmu_info, exactly for that reason. > >>> It was added in commit 523d3de41f02 ("clk: samsung: exynos5433: Add > >>> support for runtime PM"). But the clock is actually enabled only in > >>> Exynos5433 clock driver. Let's mimic what is done there in generic > >>> samsung_cmu_register_one() function, so other drivers can benefit from > >>> that `.clk_name' field. As was described above, it might be helpful not > >>> only for PM reasons, but also to prevent possible erroneous clock gating > >>> on error paths. > >>> > >>> Another way to workaround that issue would be to use CLOCK_IS_CRITICAL > >>> flag for corresponding gate clocks. But that might be not very good > >>> design decision, as we might still want to disable that bus clock, e.g. > >>> on PM suspend. > >>> > >>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > >>> --- > >>> drivers/clk/samsung/clk.c | 13 +++++++++++++ > >>> 1 file changed, 13 insertions(+) > >>> > >>> diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c > >>> index 1949ae7851b2..da65149fa502 100644 > >>> --- a/drivers/clk/samsung/clk.c > >>> +++ b/drivers/clk/samsung/clk.c > >>> @@ -357,6 +357,19 @@ struct samsung_clk_provider * __init samsung_cmu_register_one( > >>> > >>> ctx = samsung_clk_init(np, reg_base, cmu->nr_clk_ids); > >>> > >>> + /* Keep bus clock running, so it's possible to access CMU registers */ > >>> + if (cmu->clk_name) { > >>> + struct clk *bus_clk; > >>> + > >>> + bus_clk = __clk_lookup(cmu->clk_name); > >>> + if (bus_clk) { > >>> + clk_prepare_enable(bus_clk); > >>> + } else { > >>> + pr_err("%s: could not find bus clock %s\n", __func__, > >>> + cmu->clk_name); > >>> + } > >>> + } > >>> + > >> > >> Solving this problem in generic way makes sense but your solution is > >> insufficient. You skipped suspend/resume paths and in such case you > >> should remove the Exynos5433-specific code. > >> > > > > Keeping core bus clocks always running seems like a separate > > independent feature to me (not related to suspend/resume). It's > > mentioned in commit 523d3de41f02 ("clk: samsung: exynos5433: Add > > support for runtime PM") this way: > > > > "Also for each CMU there is one special parent clock, which has to > > be enabled all the time when any access to CMU registers is being > > done." > > > > Why do you think suspend/resume paths have to be implemented along > > with it? Btw, I didn't add PM ops in clk-exynos850, as PM is not > > implemented on my board yet and I can't test it. > > You can skip the runtime PM, so keep your patch almost like it is now > (in respect to Sylwester's comment about __clk_lookup). However now the > Exynos5433 will enable the clk_name twice: here and in > exynos5433_cmu_probe(). > > If you keep this approach, you need to remove duplicated part in > exynos5433_cmu_probe()... > My patch is only touching samsung_cmu_register_one(), and exynos5433_cmu_probe() doesn't call samsung_cmu_register_one(). So I don't think there can be a problem there. Or I'm missing something? samsung_cmu_register_one() is actually called from 5433 clk driver, but only from CMUs registered with CLK_OF_DECLARE(), and those are not setting .clk_name field, so my code is not affecting those either. Real problem I can see is that I can't avoid using __clk_lookup() if I implement that code in samsung_cmu_register_one(). Tried to do use clk_get(NULL, ...) instead, but it doesn't work with 1st param (dev) being NULL, because samsung_clk_register_*() functions don't register clkdev (only samsung_clk_register_fixed_rate() does), hence LIST_HEAD(clocks) is empty in clkdev.c, and clk_get() fails, when not provided with actual 'dev' param, which in turn is not present in samsung_cmu_register_one()... About using platform_driver: as I can see from clk-exynos5433.c, only CMUs which belong to Power Domains are registered as platform_driver. Rest of CMUs are registered using CLK_OF_DECLARE(), thus they don't get platform_device param. That makes it harder to avoid using __clk_lookup() inside samsung_cmu_register_one(). All that said, I feel like correct way to implement this patch would be: 1. Register all PD-capable CMUs as platform_driver in clk-exynos850 (all CMUs except CMU_TOP) 2. Move bus clock enablement code from samsung_cmu_register_one() to corresponding clk-exynos850 probe function This way I would be able to use clk_get(dev, ...) instead of __clk_lookup(), and that won't affect any existing code for sure. Code will be more unified w.r.t. how it's done in clk-exynos5433, and platform_device will be a foundation for implementing PM ops later. Taking into account how much design decisions should be done for using that in common code -- I'd say let's do that later, as a separate refactoring activity. Do you think that makes sense? Thanks! > > > > If you are suggesting moving all stuff from exynos5433_cmu_probe() > > into samsung_cmu_register_one(), it would take passing platform_device > > there, and implementing all PM related operations. I guess it's not a > > super easy task, as it would require converting clk-exynos7 to > > platform_driver for instance, and re-testing everything on exynos5433 > > and exynos7 boards (which I don't have). > > > > What do you say if I pull that code to clk-exynos850.c instead for v2? > > Refactoring (merging stuff from exynos5433_cmu_probe() into > > samsung_cmu_register_one() ) can be done later, when I add PM ops into > > clk-exynos850. > > > >> Best regards, > >> Krzysztof > > > Best regards, > Krzysztof
WARNING: multiple messages have this Message-ID (diff)
From: Sam Protsenko <semen.protsenko@linaro.org> To: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> Cc: "Sylwester Nawrocki" <s.nawrocki@samsung.com>, "Paweł Chmiel" <pawel.mikolaj.chmiel@gmail.com>, "Chanwoo Choi" <cw00.choi@samsung.com>, "Tomasz Figa" <tomasz.figa@gmail.com>, "Rob Herring" <robh+dt@kernel.org>, "Stephen Boyd" <sboyd@kernel.org>, "Michael Turquette" <mturquette@baylibre.com>, "Ryu Euiyoul" <ryu.real@samsung.com>, "Tom Gall" <tom.gall@linaro.org>, "Sumit Semwal" <sumit.semwal@linaro.org>, "John Stultz" <john.stultz@linaro.org>, "Amit Pundir" <amit.pundir@linaro.org>, devicetree <devicetree@vger.kernel.org>, "linux-arm Mailing List" <linux-arm-kernel@lists.infradead.org>, linux-clk <linux-clk@vger.kernel.org>, "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>, "Linux Samsung SOC" <linux-samsung-soc@vger.kernel.org> Subject: Re: [PATCH 1/6] clk: samsung: Enable bus clock on init Date: Wed, 6 Oct 2021 16:29:53 +0300 [thread overview] Message-ID: <CAPLW+4kq7MuF8HiY-UYvC0b8woT6G=hktJPPLD0iQ39iddmceA@mail.gmail.com> (raw) In-Reply-To: <16ee07a1-afa9-b258-8836-e96de84551db@canonical.com> On Wed, 6 Oct 2021 at 15:38, Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > On 06/10/2021 12:46, Sam Protsenko wrote: > > On Wed, 15 Sept 2021 at 11:21, Krzysztof Kozlowski > > <krzysztof.kozlowski@canonical.com> wrote: > >> > >> On 14/09/2021 17:56, Sam Protsenko wrote: > >>> By default if bus clock has no users its "enable count" value is 0. It > >>> might be actually running if it's already enabled in bootloader, but > >>> then in some cases it can be disabled by mistake. For example, such case > >>> was observed when dw_mci_probe() enabled bus clock, then failed to do > >>> something and disabled that bus clock on error path. After that even > >>> attempt to read the 'clk_summary' file in DebugFS freezed forever, as > >>> CMU bus clock ended up being disabled and it wasn't possible to access > >>> CMU registers anymore. > >>> > >>> To avoid such cases, CMU driver must increment the ref count for that > >>> bus clock by running clk_prepare_enable(). There is already existing > >>> '.clk_name' field in struct samsung_cmu_info, exactly for that reason. > >>> It was added in commit 523d3de41f02 ("clk: samsung: exynos5433: Add > >>> support for runtime PM"). But the clock is actually enabled only in > >>> Exynos5433 clock driver. Let's mimic what is done there in generic > >>> samsung_cmu_register_one() function, so other drivers can benefit from > >>> that `.clk_name' field. As was described above, it might be helpful not > >>> only for PM reasons, but also to prevent possible erroneous clock gating > >>> on error paths. > >>> > >>> Another way to workaround that issue would be to use CLOCK_IS_CRITICAL > >>> flag for corresponding gate clocks. But that might be not very good > >>> design decision, as we might still want to disable that bus clock, e.g. > >>> on PM suspend. > >>> > >>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > >>> --- > >>> drivers/clk/samsung/clk.c | 13 +++++++++++++ > >>> 1 file changed, 13 insertions(+) > >>> > >>> diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c > >>> index 1949ae7851b2..da65149fa502 100644 > >>> --- a/drivers/clk/samsung/clk.c > >>> +++ b/drivers/clk/samsung/clk.c > >>> @@ -357,6 +357,19 @@ struct samsung_clk_provider * __init samsung_cmu_register_one( > >>> > >>> ctx = samsung_clk_init(np, reg_base, cmu->nr_clk_ids); > >>> > >>> + /* Keep bus clock running, so it's possible to access CMU registers */ > >>> + if (cmu->clk_name) { > >>> + struct clk *bus_clk; > >>> + > >>> + bus_clk = __clk_lookup(cmu->clk_name); > >>> + if (bus_clk) { > >>> + clk_prepare_enable(bus_clk); > >>> + } else { > >>> + pr_err("%s: could not find bus clock %s\n", __func__, > >>> + cmu->clk_name); > >>> + } > >>> + } > >>> + > >> > >> Solving this problem in generic way makes sense but your solution is > >> insufficient. You skipped suspend/resume paths and in such case you > >> should remove the Exynos5433-specific code. > >> > > > > Keeping core bus clocks always running seems like a separate > > independent feature to me (not related to suspend/resume). It's > > mentioned in commit 523d3de41f02 ("clk: samsung: exynos5433: Add > > support for runtime PM") this way: > > > > "Also for each CMU there is one special parent clock, which has to > > be enabled all the time when any access to CMU registers is being > > done." > > > > Why do you think suspend/resume paths have to be implemented along > > with it? Btw, I didn't add PM ops in clk-exynos850, as PM is not > > implemented on my board yet and I can't test it. > > You can skip the runtime PM, so keep your patch almost like it is now > (in respect to Sylwester's comment about __clk_lookup). However now the > Exynos5433 will enable the clk_name twice: here and in > exynos5433_cmu_probe(). > > If you keep this approach, you need to remove duplicated part in > exynos5433_cmu_probe()... > My patch is only touching samsung_cmu_register_one(), and exynos5433_cmu_probe() doesn't call samsung_cmu_register_one(). So I don't think there can be a problem there. Or I'm missing something? samsung_cmu_register_one() is actually called from 5433 clk driver, but only from CMUs registered with CLK_OF_DECLARE(), and those are not setting .clk_name field, so my code is not affecting those either. Real problem I can see is that I can't avoid using __clk_lookup() if I implement that code in samsung_cmu_register_one(). Tried to do use clk_get(NULL, ...) instead, but it doesn't work with 1st param (dev) being NULL, because samsung_clk_register_*() functions don't register clkdev (only samsung_clk_register_fixed_rate() does), hence LIST_HEAD(clocks) is empty in clkdev.c, and clk_get() fails, when not provided with actual 'dev' param, which in turn is not present in samsung_cmu_register_one()... About using platform_driver: as I can see from clk-exynos5433.c, only CMUs which belong to Power Domains are registered as platform_driver. Rest of CMUs are registered using CLK_OF_DECLARE(), thus they don't get platform_device param. That makes it harder to avoid using __clk_lookup() inside samsung_cmu_register_one(). All that said, I feel like correct way to implement this patch would be: 1. Register all PD-capable CMUs as platform_driver in clk-exynos850 (all CMUs except CMU_TOP) 2. Move bus clock enablement code from samsung_cmu_register_one() to corresponding clk-exynos850 probe function This way I would be able to use clk_get(dev, ...) instead of __clk_lookup(), and that won't affect any existing code for sure. Code will be more unified w.r.t. how it's done in clk-exynos5433, and platform_device will be a foundation for implementing PM ops later. Taking into account how much design decisions should be done for using that in common code -- I'd say let's do that later, as a separate refactoring activity. Do you think that makes sense? Thanks! > > > > If you are suggesting moving all stuff from exynos5433_cmu_probe() > > into samsung_cmu_register_one(), it would take passing platform_device > > there, and implementing all PM related operations. I guess it's not a > > super easy task, as it would require converting clk-exynos7 to > > platform_driver for instance, and re-testing everything on exynos5433 > > and exynos7 boards (which I don't have). > > > > What do you say if I pull that code to clk-exynos850.c instead for v2? > > Refactoring (merging stuff from exynos5433_cmu_probe() into > > samsung_cmu_register_one() ) can be done later, when I add PM ops into > > clk-exynos850. > > > >> Best regards, > >> Krzysztof > > > Best regards, > Krzysztof _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-10-06 13:30 UTC|newest] Thread overview: 76+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-09-14 15:56 [PATCH 0/6] clk: samsung: Introduce Exynos850 SoC clock driver Sam Protsenko 2021-09-14 15:56 ` Sam Protsenko 2021-09-14 15:56 ` [PATCH 1/6] clk: samsung: Enable bus clock on init Sam Protsenko 2021-09-14 15:56 ` Sam Protsenko 2021-09-15 8:21 ` Krzysztof Kozlowski 2021-09-15 8:21 ` Krzysztof Kozlowski 2021-10-06 10:46 ` Sam Protsenko 2021-10-06 10:46 ` Sam Protsenko 2021-10-06 12:38 ` Krzysztof Kozlowski 2021-10-06 12:38 ` Krzysztof Kozlowski 2021-10-06 13:29 ` Sam Protsenko [this message] 2021-10-06 13:29 ` Sam Protsenko 2021-10-08 6:50 ` Krzysztof Kozlowski 2021-10-08 6:50 ` Krzysztof Kozlowski 2021-09-15 12:51 ` Sylwester Nawrocki 2021-09-15 12:51 ` Sylwester Nawrocki 2021-10-06 11:18 ` Sam Protsenko 2021-10-06 11:18 ` Sam Protsenko 2021-10-06 12:45 ` Krzysztof Kozlowski 2021-10-06 12:45 ` Krzysztof Kozlowski 2021-10-09 18:49 ` Sylwester Nawrocki 2021-10-09 18:49 ` Sylwester Nawrocki 2021-09-14 15:56 ` [PATCH 2/6] clk: samsung: clk-pll: Implement pll0822x PLL type Sam Protsenko 2021-09-14 15:56 ` Sam Protsenko 2021-09-15 8:24 ` Krzysztof Kozlowski 2021-09-15 8:24 ` Krzysztof Kozlowski 2021-09-15 15:59 ` Chanwoo Choi 2021-09-15 15:59 ` Chanwoo Choi 2021-09-14 15:56 ` [PATCH 3/6] clk: samsung: clk-pll: Implement pll0831x " Sam Protsenko 2021-09-14 15:56 ` Sam Protsenko 2021-09-15 8:26 ` Krzysztof Kozlowski 2021-09-15 8:26 ` Krzysztof Kozlowski 2021-09-15 16:11 ` Chanwoo Choi 2021-09-15 16:11 ` Chanwoo Choi 2021-09-14 15:56 ` [PATCH 4/6] dt-bindings: clock: Add bindings definitions for Exynos850 CMU Sam Protsenko 2021-09-14 15:56 ` Sam Protsenko 2021-09-15 8:27 ` Krzysztof Kozlowski 2021-09-15 8:27 ` Krzysztof Kozlowski 2021-09-15 16:37 ` Chanwoo Choi 2021-09-15 16:37 ` Chanwoo Choi 2021-10-05 10:28 ` Sam Protsenko 2021-10-05 10:28 ` Sam Protsenko 2021-10-06 10:49 ` Krzysztof Kozlowski 2021-10-06 10:49 ` Krzysztof Kozlowski 2021-10-06 13:31 ` Sam Protsenko 2021-10-06 13:31 ` Sam Protsenko 2021-09-21 21:10 ` Rob Herring 2021-09-21 21:10 ` Rob Herring 2021-09-14 15:56 ` [PATCH 5/6] dt-bindings: clock: Document Exynos850 CMU bindings Sam Protsenko 2021-09-14 15:56 ` Sam Protsenko 2021-09-14 21:35 ` Rob Herring 2021-09-14 21:35 ` Rob Herring 2021-09-15 8:28 ` Krzysztof Kozlowski 2021-09-15 8:28 ` Krzysztof Kozlowski 2021-10-05 11:48 ` Sam Protsenko 2021-10-05 11:48 ` Sam Protsenko 2021-09-15 16:47 ` Chanwoo Choi 2021-09-15 16:47 ` Chanwoo Choi 2021-09-14 15:56 ` [PATCH 6/6] clk: samsung: Introduce Exynos850 clock driver Sam Protsenko 2021-09-14 15:56 ` Sam Protsenko 2021-09-15 8:59 ` Krzysztof Kozlowski 2021-09-15 8:59 ` Krzysztof Kozlowski 2021-10-05 11:29 ` Sam Protsenko 2021-10-05 11:29 ` Sam Protsenko 2021-10-06 12:50 ` Krzysztof Kozlowski 2021-10-06 12:50 ` Krzysztof Kozlowski 2021-09-15 13:07 ` Sylwester Nawrocki 2021-09-15 13:07 ` Sylwester Nawrocki 2021-10-05 11:36 ` Sam Protsenko 2021-10-05 11:36 ` Sam Protsenko 2021-10-06 12:46 ` Krzysztof Kozlowski 2021-10-06 12:46 ` Krzysztof Kozlowski 2021-09-15 18:04 ` Chanwoo Choi 2021-09-15 18:04 ` Chanwoo Choi 2021-09-15 22:00 ` Sam Protsenko 2021-09-15 22:00 ` Sam Protsenko
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='CAPLW+4kq7MuF8HiY-UYvC0b8woT6G=hktJPPLD0iQ39iddmceA@mail.gmail.com' \ --to=semen.protsenko@linaro.org \ --cc=amit.pundir@linaro.org \ --cc=cw00.choi@samsung.com \ --cc=devicetree@vger.kernel.org \ --cc=john.stultz@linaro.org \ --cc=krzysztof.kozlowski@canonical.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-clk@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-samsung-soc@vger.kernel.org \ --cc=mturquette@baylibre.com \ --cc=pawel.mikolaj.chmiel@gmail.com \ --cc=robh+dt@kernel.org \ --cc=ryu.real@samsung.com \ --cc=s.nawrocki@samsung.com \ --cc=sboyd@kernel.org \ --cc=sumit.semwal@linaro.org \ --cc=tom.gall@linaro.org \ --cc=tomasz.figa@gmail.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: linkBe 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.