| Message ID | 20230315114806.3819515-2-cristian.ciocaltea@collabora.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable I2S support for RK3588/RK3588S SoCs | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/cover_letter | success | Series has a cover letter |
| conchuod/tree_selection | success | Guessed tree name to be for-next |
| conchuod/fixes_present | success | Fixes tag not required for -next series |
| conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 1 and now 1 |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/build_rv64_clang_allmodconfig | success | Errors and warnings before: 18 this patch: 18 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv64_gcc_allmodconfig | success | Errors and warnings before: 18 this patch: 18 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 3 this patch: 3 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 9 lines checked |
| conchuod/source_inline | success | Was 0 now: 0 |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | No Fixes tag |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
+Stephen On Wed, Mar 15, 2023 at 01:47:56PM +0200, Cristian Ciocaltea wrote: > Since commit df4fdd0db475 ("dt-bindings: firmware: arm,scmi: Restrict > protocol child node properties") the following dtbs_check warning is > shown: > > rk3588-rock-5b.dtb: scmi: protocol@14: Unevaluated properties are not allowed ('assigned-clock-rates', 'assigned-clocks' were unexpected) I think that's a somewhat questionable use of assigned-clock-rates. It should be located with the consumer rather than the provider IMO. The consumers of those 2 clocks are the CPU nodes. Rob
On Thu, Mar 16, 2023 at 03:34:17PM -0500, Rob Herring wrote: > +Stephen > > On Wed, Mar 15, 2023 at 01:47:56PM +0200, Cristian Ciocaltea wrote: > > Since commit df4fdd0db475 ("dt-bindings: firmware: arm,scmi: Restrict > > protocol child node properties") the following dtbs_check warning is > > shown: > > > > rk3588-rock-5b.dtb: scmi: protocol@14: Unevaluated properties are not allowed ('assigned-clock-rates', 'assigned-clocks' were unexpected) > > I think that's a somewhat questionable use of assigned-clock-rates. It > should be located with the consumer rather than the provider IMO. The > consumers of those 2 clocks are the CPU nodes. > Agreed. We definitely don't use those in the scmi clk provider driver. So NACK for the generic SCMI binding change.
On 3/17/23 00:26, Sudeep Holla wrote: > On Thu, Mar 16, 2023 at 03:34:17PM -0500, Rob Herring wrote: >> +Stephen >> >> On Wed, Mar 15, 2023 at 01:47:56PM +0200, Cristian Ciocaltea wrote: >>> Since commit df4fdd0db475 ("dt-bindings: firmware: arm,scmi: Restrict >>> protocol child node properties") the following dtbs_check warning is >>> shown: >>> >>> rk3588-rock-5b.dtb: scmi: protocol@14: Unevaluated properties are not allowed ('assigned-clock-rates', 'assigned-clocks' were unexpected) >> >> I think that's a somewhat questionable use of assigned-clock-rates. It >> should be located with the consumer rather than the provider IMO. The >> consumers of those 2 clocks are the CPU nodes. >> > > Agreed. We definitely don't use those in the scmi clk provider driver. > So NACK for the generic SCMI binding change. According to [1], "configuration of common clocks, which affect multiple consumer devices can be similarly specified in the clock provider node". That would avoid duplicating assigned-clock-rates in the CPU nodes. [1] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/clock/clock.yaml Thanks, Cristian
On Fri, Mar 17, 2023 at 4:59 AM Cristian Ciocaltea <cristian.ciocaltea@collabora.com> wrote: > > On 3/17/23 00:26, Sudeep Holla wrote: > > On Thu, Mar 16, 2023 at 03:34:17PM -0500, Rob Herring wrote: > >> +Stephen > >> > >> On Wed, Mar 15, 2023 at 01:47:56PM +0200, Cristian Ciocaltea wrote: > >>> Since commit df4fdd0db475 ("dt-bindings: firmware: arm,scmi: Restrict > >>> protocol child node properties") the following dtbs_check warning is > >>> shown: > >>> > >>> rk3588-rock-5b.dtb: scmi: protocol@14: Unevaluated properties are not allowed ('assigned-clock-rates', 'assigned-clocks' were unexpected) > >> > >> I think that's a somewhat questionable use of assigned-clock-rates. It > >> should be located with the consumer rather than the provider IMO. The > >> consumers of those 2 clocks are the CPU nodes. > >> > > > > Agreed. We definitely don't use those in the scmi clk provider driver. > > So NACK for the generic SCMI binding change. > > According to [1], "configuration of common clocks, which affect multiple > consumer devices can be similarly specified in the clock provider node". True, but in this case it's really a single consumer because it's all CPU nodes which are managed together. > That would avoid duplicating assigned-clock-rates in the CPU nodes. Wouldn't one node be sufficient? Thinking more about this, why aren't you using OPP tables to define CPU frequencies. Assigned-clocks looks like a temporary hack because you haven't done proper OPP tables. Rob
On 3/17/23 16:27, Rob Herring wrote: > On Fri, Mar 17, 2023 at 4:59 AM Cristian Ciocaltea > <cristian.ciocaltea@collabora.com> wrote: >> >> On 3/17/23 00:26, Sudeep Holla wrote: >>> On Thu, Mar 16, 2023 at 03:34:17PM -0500, Rob Herring wrote: >>>> +Stephen >>>> >>>> On Wed, Mar 15, 2023 at 01:47:56PM +0200, Cristian Ciocaltea wrote: >>>>> Since commit df4fdd0db475 ("dt-bindings: firmware: arm,scmi: Restrict >>>>> protocol child node properties") the following dtbs_check warning is >>>>> shown: >>>>> >>>>> rk3588-rock-5b.dtb: scmi: protocol@14: Unevaluated properties are not allowed ('assigned-clock-rates', 'assigned-clocks' were unexpected) >>>> >>>> I think that's a somewhat questionable use of assigned-clock-rates. It >>>> should be located with the consumer rather than the provider IMO. The >>>> consumers of those 2 clocks are the CPU nodes. >>>> >>> >>> Agreed. We definitely don't use those in the scmi clk provider driver. >>> So NACK for the generic SCMI binding change. >> >> According to [1], "configuration of common clocks, which affect multiple >> consumer devices can be similarly specified in the clock provider node". > > True, but in this case it's really a single consumer because it's all > CPU nodes which are managed together. > >> That would avoid duplicating assigned-clock-rates in the CPU nodes. > > Wouldn't one node be sufficient? Yeah, that should be fine. > Thinking more about this, why aren't you using OPP tables to define > CPU frequencies. Assigned-clocks looks like a temporary hack because > you haven't done proper OPP tables. Right, this is currently not possible since it depends on some work in progress. Thanks, Cristian
diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml index 2f7c51c75e85..10cc7ee46893 100644 --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml @@ -246,6 +246,9 @@ $defs: Channel specifier required when using OP-TEE transport and protocol has a dedicated communication channel. + assigned-clocks: true + assigned-clock-rates: true + required: - reg
Since commit df4fdd0db475 ("dt-bindings: firmware: arm,scmi: Restrict protocol child node properties") the following dtbs_check warning is shown: rk3588-rock-5b.dtb: scmi: protocol@14: Unevaluated properties are not allowed ('assigned-clock-rates', 'assigned-clocks' were unexpected) Add the missing properties. Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 3 +++ 1 file changed, 3 insertions(+)