diff mbox series

[01/11] dt-bindings: firmware: arm,scmi: Document assigned-clocks and assigned-clock-rates

Message ID 20230315114806.3819515-2-cristian.ciocaltea@collabora.com
State New
Headers
Series Enable I2S support for RK3588/RK3588S SoCs |

Commit Message

Cristian Ciocaltea March 15, 2023, 11:47 a.m. UTC 
  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(+)

Comments

Rob Herring March 16, 2023, 8:34 p.m. UTC | #1 
+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
Sudeep Holla March 16, 2023, 10:26 p.m. UTC | #2 
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.
Cristian Ciocaltea March 17, 2023, 9:59 a.m. UTC | #3 
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
Rob Herring March 17, 2023, 2:27 p.m. UTC | #4 
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
Cristian Ciocaltea March 17, 2023, 5:21 p.m. UTC | #5 
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 mbox series

Patch

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