Re: [PATCH v2 1/1] clk: samsung: exynos850: Register clocks early

From: Sam Protsenko <semen.protsenko@linaro.org>
To: Sylwester Nawrocki <snawrocki@kernel.org>
Cc: "Sumit Semwal" <sumit.semwal@linaro.org>,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-samsung-soc@vger.kernel.org,
	"Krzysztof Kozlowski" <krzysztof.kozlowski@canonical.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>,
	"Sylwester Nawrocki" <s.nawrocki@samsung.com>
Subject: Re: [PATCH v2 1/1] clk: samsung: exynos850: Register clocks early
Date: Mon, 22 Nov 2021 00:50:40 +0200	[thread overview]
Message-ID: <CAPLW+4kMK=CpRPd10hpWcY0PN72BjjA7Vh6WAvvtUHUhknEVsQ@mail.gmail.com> (raw)
In-Reply-To: <b5ac9e36-235c-906c-9f95-80dabd971be5@kernel.org>

On Sat, 20 Nov 2021 at 19:38, Sylwester Nawrocki <snawrocki@kernel.org> wrote:
>
> On 20.11.2021 17:47, Sam Protsenko wrote:
> >>> @@ -920,8 +929,12 @@ static int __init exynos850_cmu_probe(struct platform_device *pdev)
> >>>        struct device_node *np = dev->of_node;
> >>>
> >>>        info = of_device_get_match_data(dev);
> >>> -     exynos850_init_clocks(np, info->clk_regs, info->nr_clk_regs);
> >>> -     samsung_cmu_register_one(np, info);
> >>> +
> >>> +     /* Early clocks are already registered using CLK_OF_DECLARE_DRIVER() */
> >>> +     if (info != &peri_cmu_info) {
> >>> +             exynos850_init_clocks(np, info->clk_regs, info->nr_clk_regs);
> >>> +             samsung_cmu_register_one(np, info);
> >>> +     }
> >> Don't you also need to register early CMU_TOP, which provides clocks
> >> for CMU_PERI? I'm afraid it might not work properly when you register
> >> CMU_PERI clocks early and only later in probe() you enable parent clock
> >> required for the already registered clocks to be usable.
>
> > Good point, I'll do that in v2. Not sure how I missed that dependency
> > point, but thank you for noticing that. Guess it only works for me
> > because clocks are already enabled in bootloader, and I'm using
> > "clk_ignore_unused" param for now.
> >
> >> How about registering also CMU_TOP early and enabling parent clock
> >> also in OF_CLK_DECLARE init callback, i.e. using either OF_CLK_DECLARE
> >> or platform driver for a CMU?
> >>
> > If you mean doing clk_prepare_enable() for "dout_peri_bus" clock in
> > exynos850_cmu_peri_init(), I don't think it's possible. clk_get()
> > needs "struct device *dev", and we only have that in platform driver
> > probe. Trying to pass dev=NULL won't work, so that's why I'm enabling
> > parent clocks in platform driver probe.
>
> Sorry, I didn't notice it earlier, actually CMU_TOP is already being
> initialized with OF_CLK_DECLARE.
>

No worries, I forgot about that too, as you can see :-D But yeah, I'll
keep that as is then. So I'll only make parent clock enabled early.

> You could use of_clk_get() to get the clock, the consumer clock indexes
> are fixed and defined in the DT binding. There is also
> of_clk_get_by_name() which works similarly to clk_get().
>

Thanks for the suggestion! I'll go with of_clk_get_by_name(), it's
more natural as we already have parent clock name stored, so it can be
used in more general way.

This patch is superseded by next series (going to submit that soon):
    [PATCH 0/6] clk: samsung: exynos850: Clock driver improvements

>