dt-bindings: clk: vc5: Make SD/OE pin configuration properties not required

"make dtbs_check":

    arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dtb: clock-generator@6a: 'idt,shutdown' is a required property
	    From schema: Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
    arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dtb: clock-generator@6a: 'idt,output-enable-active' is a required property
	    From schema: Documentation/devicetree/bindings/clock/idt,versaclock5.yaml

Versaclock 5 clock generators can have their configuration stored in
One-Time Programmable (OTP) memory.  Hence there is no need to specify
DT properties for manual configuration if the OTP has been programmed
before.  Likewise, the Linux driver does not touch the SD/OE bits if the
corresponding properties are not specified, cfr. commit d83e561d43bc71e5
("clk: vc5: Add properties for configuring SD/OE behavior").

Reflect this in the bindings by making the "idt,shutdown" and
"idt,output-enable-active" properties not required, just like the
various "idt,*" properties in the per-output child nodes.

Fixes: 275e4e2dc0411508 ("dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 Documentation/devicetree/bindings/clock/idt,versaclock5.yaml | 2 --
 1 file changed, 2 deletions(-)

On 1/11/23 10:55, Geert Uytterhoeven wrote:
> "make dtbs_check":
> 
>     arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dtb: clock-generator@6a: 'idt,shutdown' is a required property
> 	    From schema: Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
>     arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dtb: clock-generator@6a: 'idt,output-enable-active' is a required property
> 	    From schema: Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> 
> Versaclock 5 clock generators can have their configuration stored in
> One-Time Programmable (OTP) memory.  Hence there is no need to specify
> DT properties for manual configuration if the OTP has been programmed
> before.  Likewise, the Linux driver does not touch the SD/OE bits if the
> corresponding properties are not specified, cfr. commit d83e561d43bc71e5
> ("clk: vc5: Add properties for configuring SD/OE behavior").
> 
> Reflect this in the bindings by making the "idt,shutdown" and
> "idt,output-enable-active" properties not required, just like the
> various "idt,*" properties in the per-output child nodes.

IMO we should set this stuff explicitly.

> Fixes: 275e4e2dc0411508 ("dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin")

This was intentional... not a fix IMO.

--Sean

> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  Documentation/devicetree/bindings/clock/idt,versaclock5.yaml | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> index 61b246cf5e72aa47..a5472bbfb8d1fdcc 100644
> --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> @@ -125,8 +125,6 @@ required:
>    - compatible
>    - reg
>    - '#clock-cells'
> -  - idt,shutdown
> -  - idt,output-enable-active
>  
>  allOf:
>    - if:

Hi Sean, Geert,

On Thu, 19 Jan 2023 14:27:43 -0500
Sean Anderson <sean.anderson@seco.com> wrote:

> On 1/11/23 10:55, Geert Uytterhoeven wrote:
> > "make dtbs_check":
> > 
> >     arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dtb: clock-generator@6a: 'idt,shutdown' is a required property
> > 	    From schema: Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> >     arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dtb: clock-generator@6a: 'idt,output-enable-active' is a required property
> > 	    From schema: Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> > 
> > Versaclock 5 clock generators can have their configuration stored in
> > One-Time Programmable (OTP) memory.  Hence there is no need to specify
> > DT properties for manual configuration if the OTP has been programmed
> > before.  Likewise, the Linux driver does not touch the SD/OE bits if the
> > corresponding properties are not specified, cfr. commit d83e561d43bc71e5
> > ("clk: vc5: Add properties for configuring SD/OE behavior").
> > 
> > Reflect this in the bindings by making the "idt,shutdown" and
> > "idt,output-enable-active" properties not required, just like the
> > various "idt,*" properties in the per-output child nodes.  
> 
> IMO we should set this stuff explicitly.

I took a moment to think better about this and I think I get your point
Sean in preferring that the hardware is described in detail.

However I'm still leaning towards approving Geert's proposal.

I'm based on the principle that DT is there to describe the aspects of
the hardware that the software needs _and_ it is unable to discover by
itself.

Based on that, does the software need to know SD/OR configuration? If
they are already written in the OTP then it doesn't. Also if the chip
default is the use that is implemented on the board, it also doesn't
(like lots of optional properties, especially when in most cases a
given chip is used in the default configuration but not always).

To some extent, writing settings in an OTP is similar to producing a
different chip where these values are hard-coded and not configured.

I'm wondering whether Geert has a practical example of a situation
where it is better to have these properties optional.

Best regards.

On 1/24/23 03:28, Geert Uytterhoeven wrote:
> Hi Luca,
> 
> On Tue, Jan 24, 2023 at 9:12 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
>> On Thu, 19 Jan 2023 14:27:43 -0500
>> Sean Anderson <sean.anderson@seco.com> wrote:
>> > On 1/11/23 10:55, Geert Uytterhoeven wrote:
>> > > "make dtbs_check":
>> > >
>> > >     arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dtb: clock-generator@6a: 'idt,shutdown' is a required property
>> > >         From schema: Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
>> > >     arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dtb: clock-generator@6a: 'idt,output-enable-active' is a required property
>> > >         From schema: Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
>> > >
>> > > Versaclock 5 clock generators can have their configuration stored in
>> > > One-Time Programmable (OTP) memory.  Hence there is no need to specify
>> > > DT properties for manual configuration if the OTP has been programmed
>> > > before.  Likewise, the Linux driver does not touch the SD/OE bits if the
>> > > corresponding properties are not specified, cfr. commit d83e561d43bc71e5
>> > > ("clk: vc5: Add properties for configuring SD/OE behavior").
>> > >
>> > > Reflect this in the bindings by making the "idt,shutdown" and
>> > > "idt,output-enable-active" properties not required, just like the
>> > > various "idt,*" properties in the per-output child nodes.
>> >
>> > IMO we should set this stuff explicitly.
>>
>> I took a moment to think better about this and I think I get your point
>> Sean in preferring that the hardware is described in detail.
>>
>> However I'm still leaning towards approving Geert's proposal.
>>
>> I'm based on the principle that DT is there to describe the aspects of
>> the hardware that the software needs _and_ it is unable to discover by
>> itself.
>>
>> Based on that, does the software need to know SD/OR configuration? If
>> they are already written in the OTP then it doesn't. Also if the chip
>> default is the use that is implemented on the board, it also doesn't
>> (like lots of optional properties, especially when in most cases a
>> given chip is used in the default configuration but not always).
> 
> These clock generators are typically programmed through OTP because
> (at least some of) the configuration is needed to boot the board.

Actually, I found that with these properties added, there is no need to
program the OTP (at least for my use-case). This is great because it
removes a step during manufacture and you don't have to worry about
getting something wrong.

>> To some extent, writing settings in an OTP is similar to producing a
>> different chip where these values are hard-coded and not configured.
> 
> Indeed. Except that here they can still be overwritten by software
> later.
> 
> The latter is an inherent danger, and is probably the reason why Sean
> wants to make it explicit: if the configuration is ever changed by
> software, and the system is restarted without resetting the clock
> generator (at least 5P49V6901A does not have a reset line), or using
> kexec/kdump, the newly booted kernel does not know the intended
> settings, and won't restore the correct configuration.
>
>> I'm wondering whether Geert has a practical example of a situation
>> where it is better to have these properties optional.
> 
> My issue was that these properties were introduced long after the
> initial bindings, hence pre-existing DTS does not have them.
> Yes, we can add them, but then we have to read out the OTP-programmed
> settings first. If that's the way to go, I can look into that, though...

FWIW I think there's no need to update existing bindings which don't
have this property. The required aspect is mainly a reminder for new
device trees.

--Sean

Quoting Sean Anderson (2023-01-24 08:23:45)
> On 1/24/23 03:28, Geert Uytterhoeven wrote:
> > Hi Luca,
> > 
> > On Tue, Jan 24, 2023 at 9:12 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> >> On Thu, 19 Jan 2023 14:27:43 -0500
> >> Sean Anderson <sean.anderson@seco.com> wrote:
> >> > On 1/11/23 10:55, Geert Uytterhoeven wrote:
> >
> >> I'm wondering whether Geert has a practical example of a situation
> >> where it is better to have these properties optional.
> > 
> > My issue was that these properties were introduced long after the
> > initial bindings, hence pre-existing DTS does not have them.
> > Yes, we can add them, but then we have to read out the OTP-programmed
> > settings first. If that's the way to go, I can look into that, though...
> 
> FWIW I think there's no need to update existing bindings which don't
> have this property. The required aspect is mainly a reminder for new
> device trees.
> 

Is there any resolution on this thread? I'm dropping this patch from my
queue.