2021-02-23 17:03:08

by Álvaro Fernández Rojas

[permalink] [raw]
Subject: [PATCH v3 1/2] dt-bindings: rng: bcm2835: document reset support

Some devices may need to perform a reset before using the RNG, such as the
BCM6368.

Signed-off-by: Álvaro Fernández Rojas <[email protected]>
---
v3: make resets required if brcm,bcm6368-rng.
v2: document reset support.

.../devicetree/bindings/rng/brcm,bcm2835.yaml | 17 +++++++++++++++++
1 file changed, 17 insertions(+)

diff --git a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
index c147900f9041..11c23e1f6988 100644
--- a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
+++ b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
@@ -37,6 +37,21 @@ required:

additionalProperties: false

+if:
+ properties:
+ compatible:
+ enum:
+ - brcm,bcm6368-rng
+then:
+ properties:
+ resets:
+ maxItems: 1
+ required:
+ - resets
+else:
+ properties:
+ resets: false
+
examples:
- |
rng@7e104000 {
@@ -58,4 +73,6 @@ examples:

clocks = <&periph_clk 18>;
clock-names = "ipsec";
+
+ resets = <&periph_rst 4>;
};
--
2.20.1


2021-03-04 17:51:30

by Álvaro Fernández Rojas

[permalink] [raw]
Subject: Re: [PATCH v3 1/2] dt-bindings: rng: bcm2835: document reset support

Hi Nicolas,

> El 4 mar 2021, a las 14:30, Nicolas Saenz Julienne <[email protected]> escribió:
>
> On Thu, 2021-03-04 at 13:18 +0100, Álvaro Fernández Rojas wrote:
>>
>>> El 4 mar 2021, a las 13:07, Nicolas Saenz Julienne <[email protected]> escribió:
>>>
>>> Hi Alvaro,
>>>
>>> On Tue, 2021-02-23 at 18:00 +0100, Álvaro Fernández Rojas wrote:
>>>> Some devices may need to perform a reset before using the RNG, such as the
>>>> BCM6368.
>>>>
>>>> Signed-off-by: Álvaro Fernández Rojas <[email protected]>
>>>> ---
>>>> v3: make resets required if brcm,bcm6368-rng.
>>>> v2: document reset support.
>>>>
>>>> .../devicetree/bindings/rng/brcm,bcm2835.yaml | 17 +++++++++++++++++
>>>> 1 file changed, 17 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
>>>> index c147900f9041..11c23e1f6988 100644
>>>> --- a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
>>>> +++ b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
>>>> @@ -37,6 +37,21 @@ required:
>>>>
>>>>
>>>>
>>>> additionalProperties: false
>>>
>>> I can't claim I fully understand all the meta stuff in shemas, so I generally
>>> just follow the patterns already available out there.
>>
>> Well, that makes two of us :).
>>
>>> That said, shoudln't this be at the end, just before the examples?
>>
>> I don’t know but I can move it there ¯\_(ツ)_/¯
>
> Yes please do. See commit 7f464532b05 ("dt-bindings: Add missing
> 'additionalProperties: false'") which expands on why it is needed, and why it
> should be at the end.
>
>>> Maybe the cause of that odd warning
>>> you got there?
>>
>> Which odd warning?
>
> The one pointed out by Rob Herring's script, which I can reproduce too BTW. On
> the other hand I can't really tell what's wrong right away.

Well, I can’t reproduce that locally and I don’t know what’s wrong either, I think that the best option is to remove the full if block.

>
>> I don’t get any warnings when running (or at least warnings related to rig, because I get warnings related to other yamls):
>> make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
>>
>>>
>>>> +if:
>>>> + properties:
>>>> + compatible:
>>>> + enum:
>>>> + - brcm,bcm6368-rng
>>>> +then:
>>>> + properties:
>>>> + resets:
>>>> + maxItems: 1
>>>> + required:
>>>> + - resets
>>>
>>> I belive you can't really make a property required when the bindings for
>>> 'brcm,bcm6368-rng' were already defined. This will break the schema for those
>>> otherwise correct devicetrees.
>>
>> Why not?
>> Wouldn’t just be required for brcm,bcm6368-rng?
>
> Well, do all 'brcm,bcm6368-rng' users absolutely need the reset controller? If
> so, the original binding is wrong, which should be mentioned and a 'Fixes:' tag
> should be added to the commit message. Otherwise, the requirement is optional.

I’m not sure about this...

>
> Regards,
> Nicolas

Best regards,
Álvaro.

2021-03-04 20:01:47

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH v3 1/2] dt-bindings: rng: bcm2835: document reset support

On Thu, Mar 4, 2021 at 6:07 AM Nicolas Saenz Julienne
<[email protected]> wrote:
>
> Hi Alvaro,
>
> On Tue, 2021-02-23 at 18:00 +0100, Álvaro Fernández Rojas wrote:
> > Some devices may need to perform a reset before using the RNG, such as the
> > BCM6368.
> >
> > Signed-off-by: Álvaro Fernández Rojas <[email protected]>
> > ---
> > v3: make resets required if brcm,bcm6368-rng.
> > v2: document reset support.
> >
> > .../devicetree/bindings/rng/brcm,bcm2835.yaml | 17 +++++++++++++++++
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
> > index c147900f9041..11c23e1f6988 100644
> > --- a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
> > +++ b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
> > @@ -37,6 +37,21 @@ required:
> >
> >
> > additionalProperties: false
>
> I can't claim I fully understand all the meta stuff in shemas, so I generally
> just follow the patterns already available out there. That said, shoudln't this
> be at the end, just before the examples? Maybe the cause of that odd warning
> you got there?

Yes, 'resets' needs to be defined under 'properties' as
additionalProperties can't 'see' into if/then schemas. The top level
needs to define everything and then if/then schema just add additional
constraints.

>
> > +if:
> > + properties:
> > + compatible:
> > + enum:
> > + - brcm,bcm6368-rng
> > +then:
> > + properties:
> > + resets:
> > + maxItems: 1
> > + required:
> > + - resets
>
> I belive you can't really make a property required when the bindings for
> 'brcm,bcm6368-rng' were already defined. This will break the schema for those
> otherwise correct devicetrees.

Right, unless not having the property meant the device never would have worked.

Rob