MIME-Version: 1.0
References: <CGME20230308230935eucas1p1e919f4d4b020e3386ce0eac8b4c8d299@eucas1p1.samsung.com>
 <20230308230931.27261-1-semen.protsenko@linaro.org> <d1175c3e-301d-1cbc-607c-e94051780806@samsung.com>
 <611fe922-1937-d37d-a2ce-cc0a13aed9e0@linaro.org>
In-Reply-To: <611fe922-1937-d37d-a2ce-cc0a13aed9e0@linaro.org>
From:   Sam Protsenko <semen.protsenko@linaro.org>
Date:   Mon, 20 Mar 2023 12:57:14 -0500
Message-ID: <CAPLW+4=Yd0KfaMp+oSA=rOrqJfUotHjB=QOKpaPitMh3V2fMrA@mail.gmail.com>
Subject: Re: [PATCH 0/6] soc: samsung: pm_domains: Add Exynos850 support
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        Marek Szyprowski <m.szyprowski@samsung.com>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Alim Akhtar <alim.akhtar@samsung.com>,
        Chanwoo Choi <cw00.choi@samsung.com>,
        Chanho Park <chanho61.park@samsung.com>,
        David Virag <virag.david003@gmail.com>,
        linux-arm-kernel@lists.infradead.org,
        linux-samsung-soc@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, 10 Mar 2023 at 08:41, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 09/03/2023 11:12, Marek Szyprowski wrote:
> > Hi Sam,
> >
> > On 09.03.2023 00:09, Sam Protsenko wrote:
> >> Power Domains in Exynos850 are not really different from other Exynos
> >> platforms. Enabling Exynos850 support in the PD driver is really just a
> >> matter of adding:
> >>
> >>      static const struct exynos_pm_domain_config exynos850_cfg = {
> >>          .local_pwr_cfg = 0x1,
> >>      };
> >>
> >> to the driver. But in the face of recent developments, e.g. this patch:
> >>
> >>      arm64: dts: exynos: move MIPI phy to PMU node in Exynos5433
> >>
> >> it looked logical to rework the PD driver a bit to support its nesting
> >> under the PMU node, while adding Exynos850 support to it. Initially I
> >> only wanted to add syscon regmap support via some dedicated property,
> >> but pulling PD nodes under the PMU syscon looks like more correct way.
> >
> > Frankly speaking if you are changing this, you can go even further.
> > Simply make PMU node a PM domain provider and specify the power domain
> > as a phandle parameter. This is how it should have been done from the
> > beginning, but for some unknown reasons wasn't. There is really no need
> > to have a separate node for each power domain. This will also move
> > implementation details to the PMU / power domain drivers and it will
> > make it much easier to extend/modify it in the future. IMHO same applies
> > for PHY nodes.
>
> I agree. The "samsung,pd-index" property is not a correct approach.
> Either you use address space or not. If not, then this should be part of
> power domain provider, which is also matching most of other SoC
> architectures.
>

Did a bit of research, looked at how it's implemented on other
platforms. Before I start reworking it, want to check with you on a
couple of decisions, to avoid unnecessary resubmissions later, if it's
ok:

1. Instead of actually merging PD driver into PMU driver, guess it
might be better to create a new power-controller driver (e.g.
drivers/soc/samsung/exynos-power.c). This is how it's implemented for
the most of platforms, and this way we can neatly separate it from
what we already have in the PMU driver (not really power controller
related things). This way, in device tree we'll have a
power-controller node under PMU node, and this node can be referenced
further as a phandle in power-domains properties of users.

2. After moving PD implementation into a new power-controller driver
(with new compatibility string), the old one (pm_domains.c) should be
probably removed. Is it reasonable, e.g. from point of view of
compatibility with out-of-tree (downstream) dts's? Also, if I remove
the PD driver, probably all existing (upstream) Exynos dts's should be
reworked to use the new power-controller compatibility string?

3. Where to keep offsets for each power domain (inside of PMU register
area). Can be done in dts (a separate child node of power-controller
for each power domain), or in the power-controller driver. I saw both
of those ways for different platforms actually. But I guess offsets
are more like internal details, and should be kept inside the driver,
for each supported SoC.

4. Specifying particular power domain in power-domains property. Guess
the best way would be to have some indexes defined in dt-bindings
header, and use those like this:

        power-domains = <&power_controller EXYNOS850_PD_G3D>;

   Those constants can be also used then in the driver, to keep PD
offsets in the array, etc. Another way would be to use reg offsets,
but indices look better: can provide more flexibility in the driver in
future, e.g. if we'd need to add some more details other that offsets
later.

Please let me know what you think. At the moment I have to switch to
another task temporarily. When I get back to this one, discussing the
above items would help me a great deal.

Thanks!

>
> Best regards,
> Krzysztof
>