| Message ID | 20240708194808.1819185-2-gnstark@salutedevices.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | amlogic SoC's power-domains fixes | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
On 08/07/2024 21:48, George Stark wrote: > SPI Flash Controller has dedicated power domain so make the > corresponding property required. That's an ABI break. Being part of power domain does not necessarily mean it must be required, so your commit msg lacks rationale. Especially on the ABI part. Best regards, Krzysztof
On Mon, 08 Jul 2024 22:48:05 +0300, George Stark wrote: > SPI Flash Controller has dedicated power domain so make the > corresponding property required. > > Signed-off-by: George Stark <gnstark@salutedevices.com> > --- > Documentation/devicetree/bindings/spi/amlogic,a1-spifc.yaml | 4 ++++ > 1 file changed, 4 insertions(+) > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/spi/amlogic,a1-spifc.example.dtb: spi@fd000400: Unevaluated properties are not allowed ('power-domains' was unexpected) from schema $id: http://devicetree.org/schemas/spi/amlogic,a1-spifc.yaml# doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240708194808.1819185-2-gnstark@salutedevices.com 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.
Hello Krzysztof, Neil, Rob Thanks for your reviews. I'd just like to clarify things a little on that matter. On A1 most of periphery has either dedicated power domain (ir, spifc, spi, cpu etc) or several peripheries share single PD e.g. pwm, temp sensor, i2c, adc share PD 12. Appropriate PD must be turned on before the periphery is touched and be on while periphery is used. Experiments confirm it. So power-domains property in dts nodes is really required. It makes kernel to control PDs when bound periphery is used. If I understand bindings thing correctly we should make power-domains property required in bindings too in that case, at least for those peripheries which has *a1* compatibles (some peripheries use compatible from older SoCs without PDs). What you think? On 7/8/24 22:51, Krzysztof Kozlowski wrote: > On 08/07/2024 21:48, George Stark wrote: >> SPI Flash Controller has dedicated power domain so make the >> corresponding property required. > > That's an ABI break. Being part of power domain does not necessarily > mean it must be required, so your commit msg lacks rationale. Especially > on the ABI part. > > > > Best regards, > Krzysztof >
On 10/07/2024 18:23, George Stark wrote: > Hello Krzysztof, Neil, Rob > > Thanks for your reviews. > > I'd just like to clarify things a little on that matter. On A1 most of > periphery has either dedicated power domain (ir, spifc, spi, cpu etc) or > several peripheries share single PD e.g. pwm, temp sensor, i2c, adc > share PD 12. Appropriate PD must be turned on before the periphery is > touched and be on while periphery is used. Experiments confirm it. So > power-domains property in dts nodes is really required. It makes kernel > to control PDs when bound periphery is used. > > If I understand bindings thing correctly we should make power-domains > property required in bindings too in that case, at least for those > peripheries which has *a1* compatibles (some peripheries use compatible > from older SoCs without PDs). > > What you think? Yes, but if the compatible has already been defined without a required power-domain, then it's too late to mark it as required since it's an ABI break. So next time a new compatible is added, it's the appropriate time to mark it as required. But in any case it's not a big deal because if the power-domain is not specified it won't work, like if you use wrong register address or the wrong interrupt line. The fact power-domain can be specified is enough. Neil > > On 7/8/24 22:51, Krzysztof Kozlowski wrote: >> On 08/07/2024 21:48, George Stark wrote: >>> SPI Flash Controller has dedicated power domain so make the >>> corresponding property required. >> >> That's an ABI break. Being part of power domain does not necessarily >> mean it must be required, so your commit msg lacks rationale. Especially >> on the ABI part. >> >> >> >> Best regards, >> Krzysztof >> >
diff --git a/Documentation/devicetree/bindings/spi/amlogic,a1-spifc.yaml b/Documentation/devicetree/bindings/spi/amlogic,a1-spifc.yaml index 043879b434ac..e704aff8a862 100644 --- a/Documentation/devicetree/bindings/spi/amlogic,a1-spifc.yaml +++ b/Documentation/devicetree/bindings/spi/amlogic,a1-spifc.yaml @@ -30,15 +30,19 @@ required: - compatible - reg - clocks + - power-domains unevaluatedProperties: false examples: - | + #include <dt-bindings/power/meson-a1-power.h> + spi@fd000400 { compatible = "amlogic,a1-spifc"; reg = <0xfd000400 0x290>; clocks = <&clkc_clkid_spifc>; #address-cells = <1>; #size-cells = <0>; + power-domains = <&pwrc PWRC_SPIFC_ID>; };
SPI Flash Controller has dedicated power domain so make the corresponding property required. Signed-off-by: George Stark <gnstark@salutedevices.com> --- Documentation/devicetree/bindings/spi/amlogic,a1-spifc.yaml | 4 ++++ 1 file changed, 4 insertions(+)