Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3d4b66b7-b86b-3ff2-1076-33a16a8014f8@linaro.org>
Date:   Wed, 22 Mar 2023 19:52:01 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH 0/6] soc: samsung: pm_domains: Add Exynos850 support
Content-Language: en-US
To:     Sam Protsenko <semen.protsenko@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
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>
 <CAPLW+4=Yd0KfaMp+oSA=rOrqJfUotHjB=QOKpaPitMh3V2fMrA@mail.gmail.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <CAPLW+4=Yd0KfaMp+oSA=rOrqJfUotHjB=QOKpaPitMh3V2fMrA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 20/03/2023 18:57, Sam Protsenko wrote:
> 
> 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.

Whether you want separate driver is different from wanting separate
device node in DT. About the Devicetree, the PMU node is the power
management unit responsible for:
1. power domains,
2. system power.

Thus separate device node rather does not fit here. This is one bigger
device which can have sub-blocks (e.g. system-reboot), but power domain
handling does not look like separate from it. It is its core.

Now about driver - whatever creates readable and maintainable code :)

> 
> 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? 

You must not break the ABI, so existing bindings and driver must stay.
The power domain cells in PMU node will be the trigger for using new code.

> 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. 

Just like in clock or reset drivers, this should be in the driver.

> I saw both
> of those ways for different platforms actually. 

Then share examples, because maybe I think about something else...

> 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>;

Yes.

> 
>    Those constants can be also used then in the driver, to keep PD
> offsets in the array, etc.

Yes.

> 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.

Right, use logical IDs and driver will do the translation.

> 
> 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.


Sure, thanks for the work, keep safe and strong!

Best regards,
Krzysztof