| Message ID | 20241213-contrib-pg-cpu-hotplug-suspend2ram-fixes-v1-v1-2-c72978f63713@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Fix Google Tensor GS101 CPU hotplug support | expand |
On Fri, 13 Dec 2024 16:44:39 +0000, Peter Griffin wrote: > To avoid dtschema warnings allow google,gs101-pmu to have > two reg regions. > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > --- > I don't really like this patch, but also didn't want to submit the series > with a dtschema warning ;-) > > Possibly a better solution is when Robs patch > `mfd: syscon: Allow syscon nodes without a "syscon" compatible` [1] > > gets updated with a v2, we could remove syscon compatible from > gs101.dtsi (an ABI issue). If I understood his patch correctly, > it would mean this yaml update would then no longer be required. > > Let me know your thoughts > > [1] https://lore.kernel.org/lkml/20241211-syscon-fixes-v1-0-b5ac8c219e96@kernel.org/T/#m5ad1ed5c69f693d2a5cc54342a87fbdf3df756d2 > --- > Documentation/devicetree/bindings/mfd/syscon-common.yaml | 10 ++++++++++ > 1 file changed, 10 insertions(+) > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: ./Documentation/devicetree/bindings/mfd/syscon-common.yaml:68:3: [error] syntax error: expected <block end>, but found '?' (syntax) ./Documentation/devicetree/bindings/mfd/syscon-common.yaml:69:5: [warning] wrong indentation: expected 6 but found 4 (indentation) dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/syscon-common.yaml: ignoring, error parsing file make[2]: *** Deleting file 'Documentation/devicetree/bindings/mfd/syscon-common.example.dts' Documentation/devicetree/bindings/mfd/syscon-common.yaml:68:3: expected <block end>, but found '?' make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/mfd/syscon-common.example.dts] Error 1 make[2]: *** Waiting for unfinished jobs.... ./Documentation/devicetree/bindings/mfd/syscon-common.yaml:68:3: expected <block end>, but found '?' make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1506: dt_binding_check] Error 2 make: *** [Makefile:251: __sub-make] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241213-contrib-pg-cpu-hotplug-suspend2ram-fixes-v1-v1-2-c72978f63713@linaro.org The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Mon, Dec 16, 2024 at 8:19 AM Rob Herring <robh@kernel.org> wrote: > > On Fri, Dec 13, 2024 at 04:44:39PM +0000, Peter Griffin wrote: > > To avoid dtschema warnings allow google,gs101-pmu to have > > two reg regions. > > It's not a "simple" syscon if you have 2 regions, so put it in its own > schema doc. NM, I see it already does. If you keep 'syscon', then 'maxItems: 1' will probably need to move to syscon.yaml. Rob
On 13/12/2024 17:44, Peter Griffin wrote: > To avoid dtschema warnings allow google,gs101-pmu to have > two reg regions. > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > --- > I don't really like this patch, but also didn't want to submit the series > with a dtschema warning ;-) > > Possibly a better solution is when Robs patch > `mfd: syscon: Allow syscon nodes without a "syscon" compatible` [1] PMU which spans over two blocks is not a simple syscon. These would be two syscon devices. If you request regmap from such syscon, which regmap you get? I am not sure whether the PMU is really split here. Usually the main PMU was only one and additional blocks called PMU were somehow specialized per each IP block. Maybe you have here two devices, maybe only one. If it is only one, then it is not a syscon anymore, IMO. Best regards, Krzysztof
Hi Krzysztof, Thanks for your review feedback, it is much appreciated! On Sun, 22 Dec 2024 at 14:24, Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On 13/12/2024 17:44, Peter Griffin wrote: > > To avoid dtschema warnings allow google,gs101-pmu to have > > two reg regions. > > > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > > --- > > I don't really like this patch, but also didn't want to submit the series > > with a dtschema warning ;-) > > > > Possibly a better solution is when Robs patch > > `mfd: syscon: Allow syscon nodes without a "syscon" compatible` [1] > > PMU which spans over two blocks is not a simple syscon. These would be > two syscon devices. > > If you request regmap from such syscon, which regmap you get? That is a good point, if other drivers in the future need access to the pmu-intr-gen registers then it would be good if this was modelled as its own syscon device. Another point to note is that only the PMU registers need the custom regmap registered in exynos-pmu, the PMU_INTR_GEN register region works with normal syscon / mmio accesses, so it would be a different regmap. > > I am not sure whether the PMU is really split here. Usually the main PMU > was only one and additional blocks called PMU were somehow specialized > per each IP block. PMU_INTR_GEN has its own entry in the memory map, so in that respect it's a "device" (it has its own 65k SFR region). PMU: Base Address 0x1746_0000 PMU_INTR_GEN: Base Address 0x1747_0000 The documentation isn't particularly detailed on PMU_INTR_GEN. In one place it says "One PMU interrupt generator for handshaking between PMU through interrupts". In another, "PMU and PMU_INTR_GEN are for Power management." and then we have the register names where the description it really an expanded version of the register name e.g. Register: GRP#_INTR_BID_ENABLE Description: Interrupt Bid Enable Reset Value: 0x0 Things might be a bit clearer if I had access to the firmware code on the other side of this PMU handshaking which I believe is the APM, but sadly I don't. > > Maybe you have here two devices, maybe only one. If it is only one, then > it is not a syscon anymore, IMO. I was going to suggest modelling PMU_INTR_GEN as its own sycon node, and then either: - 1) Updating exynos-pmu driver to additionally take a phandle to pmu-intr-gen syscon, and register the hotplug callbacks. or 2) Create a new driver named something like exynos-pm or exynos-cpupm which obtains the PMU regmap and also a phandle to PMU_INTR_GEN syscon, and register the call backs. Is there any preference from your side over approach 1 or 2, or maybe something else entirely? Thanks, Peter
On 30/12/2024 10:10, Peter Griffin wrote: > >> >> Maybe you have here two devices, maybe only one. If it is only one, then >> it is not a syscon anymore, IMO. > > I was going to suggest modelling PMU_INTR_GEN as its own sycon node, > and then either: - > > 1) Updating exynos-pmu driver to additionally take a phandle to > pmu-intr-gen syscon, and register the hotplug callbacks. > > or > > 2) Create a new driver named something like exynos-pm or exynos-cpupm > which obtains the PMU regmap and also a phandle to PMU_INTR_GEN > syscon, and register the call backs. > > Is there any preference from your side over approach 1 or 2, or maybe > something else entirely? No preference, choose whatever results in simpler or more readable code. Option 1 assumes that exynos-pmu on GS101 will drop the "syscon" compatible, although it still might expose syscon through drivers. Just the standard binding syscon does not feel fit here. I don't have the hardware/user manual, so I don't know what PMU_INTR_GEN really is. GS downstream code has something like PMUCAL, which looks like separate device. Best regards, Krzysztof
Hi Krzysztof, Thanks for your feedback! On Fri, 3 Jan 2025 at 17:14, Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On 30/12/2024 10:10, Peter Griffin wrote: > > > >> > >> Maybe you have here two devices, maybe only one. If it is only one, then > >> it is not a syscon anymore, IMO. > > > > I was going to suggest modelling PMU_INTR_GEN as its own sycon node, > > and then either: - > > > > 1) Updating exynos-pmu driver to additionally take a phandle to > > pmu-intr-gen syscon, and register the hotplug callbacks. > > > > or > > > > 2) Create a new driver named something like exynos-pm or exynos-cpupm > > which obtains the PMU regmap and also a phandle to PMU_INTR_GEN > > syscon, and register the call backs. > > > > Is there any preference from your side over approach 1 or 2, or maybe > > something else entirely? > > No preference, choose whatever results in simpler or more readable code. > > Option 1 assumes that exynos-pmu on GS101 will drop the "syscon" > compatible, although it still might expose syscon through drivers. Just > the standard binding syscon does not feel fit here. I agree we should drop syscon compatible for gs101 as it requires a "special" regmap. However other Exynos based SoCs will likely want to re-use this pmu_intr_gen CPU pm code and they will likely have syscon compatible in their exynos-pmu node (as protecting PMU registers from Linux AFAIK was a Google hardening measure). So just to clarify, dropping syscon compatible on option 1 is because it's gs101 "special" regmap, or because exynos-pmu node now references additional pmu_intr_gen syscon? > > I don't have the hardware/user manual, so I don't know what PMU_INTR_GEN > really is. There isn't much description in the manual, but AIUI pmu_intr_gen is just a way for the OS to trigger an interrupt so that the APM programs the PMU registers instead of the OS programming PMU registers directly. It looks like the system could also be configured to not use APM (it would need different firmware), in which case the OS would just program PMU registers directly. I only see these GRP(x)_INTR_BID_ENABLE / GRP(x)_INTR_BID_CLEAR registers mentioned in downstream code in the context of flexpmu_cal_system_gs101.h (which is basically lists of registers to program for different power/sleep modes - which looks like what exynos-pmu is currently doing for older chipsets) and flexpmu_cal_cpu_gs101.h (which is used for cpu on/off) related things. So even if I split the CPU pm parts into a separate driver, it looks like programming pmu-intr-gen regs would still be required to enter/exit sleep modes. With that in mind I think it seems more natural to grow the exynos-pmu node & driver. > GS downstream code has something like PMUCAL, which looks > like separate device. PMUCAL is just the PMU registers with a whole bunch of layering. I believe CAL just stands for Cpu Abstraction Layer and seems to be used in downstream Samsung driver code when they have a bunch of "generic" driver code and then what appears to be a lot of automatically generated header files for a particular SoC for reading/writing all the SFRs. The CAL suffix is used for PMU and also for clocks (CMU). Most of the PMUCAL code is just accessing PMU and pmu-intr-gen registers (if APM is configured). There are some places like flexpmu_cal_system_gs101.h where it seems to be used as a convenient place to read/write registers all over the SoC memory map (CMU, SYSREG* etc). So unpicking that into all the various subsystems will be interesting. Thanks, Peter
diff --git a/Documentation/devicetree/bindings/mfd/syscon-common.yaml b/Documentation/devicetree/bindings/mfd/syscon-common.yaml index 451cbad467a3..9cd9739d5e97 100644 --- a/Documentation/devicetree/bindings/mfd/syscon-common.yaml +++ b/Documentation/devicetree/bindings/mfd/syscon-common.yaml @@ -59,6 +59,16 @@ allOf: compatible: minItems: 3 maxItems: 5 + - if: + properties: + compatible: + contains: + const: + - google,gs101-pmu + then: + properties: + reg: + maxItems: 2 additionalProperties: true
To avoid dtschema warnings allow google,gs101-pmu to have two reg regions. Signed-off-by: Peter Griffin <peter.griffin@linaro.org> --- I don't really like this patch, but also didn't want to submit the series with a dtschema warning ;-) Possibly a better solution is when Robs patch `mfd: syscon: Allow syscon nodes without a "syscon" compatible` [1] gets updated with a v2, we could remove syscon compatible from gs101.dtsi (an ABI issue). If I understood his patch correctly, it would mean this yaml update would then no longer be required. Let me know your thoughts [1] https://lore.kernel.org/lkml/20241211-syscon-fixes-v1-0-b5ac8c219e96@kernel.org/T/#m5ad1ed5c69f693d2a5cc54342a87fbdf3df756d2 --- Documentation/devicetree/bindings/mfd/syscon-common.yaml | 10 ++++++++++ 1 file changed, 10 insertions(+)