| Message ID | 20230308230931.27261-1-semen.protsenko@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | soc: samsung: pm_domains: Add Exynos850 support | expand |
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. Best regards
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. Best regards, Krzysztof
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 >
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
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. This patch series provides next changes: 1. Make it possible for PD nodes to be children of PMU 2. Add Exynos850 support to PD driver 3. A bit of refactoring in PD driver 4. Corresponding changes to dt-bindings Dependencies inside of the series: - patch #2 depends on patch #1 - patch #6 depends on patch #1 - patches 3,4,5,6 should be applied in the same order as in the series Sam Protsenko (6): dt-bindings: power: pd-samsung: Add Exynos850 support dt-bindings: power: pd-samsung: Allow pd nodes to be children of PMU soc: samsung: pm_domains: Extract DT handling into a separate function soc: samsung: pm_domains: Implement proper I/O operations soc: samsung: pm_domains: Allow PD to be a child of PMU syscon soc: samsung: pm_domains: Add Exynos850 support .../devicetree/bindings/power/pd-samsung.yaml | 12 +- MAINTAINERS | 1 + drivers/soc/samsung/Kconfig | 1 + drivers/soc/samsung/pm_domains.c | 132 +++++++++++++++--- .../power/samsung,exynos850-power.h | 17 +++ 5 files changed, 142 insertions(+), 21 deletions(-) create mode 100644 include/dt-bindings/power/samsung,exynos850-power.h