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

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

[Geert Uytterhoeven]

"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(-)

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:
-- 
2.34.1

Re: [PATCH] dt-bindings: clk: vc5: Make SD/OE pin configuration properties not required

[Krzysztof Kozlowski]

On 11/01/2023 16: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
> 

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH] dt-bindings: clk: vc5: Make SD/OE pin configuration properties not required

[Luca Ceresoli]

Hi Geert,

On Wed, 11 Jan 2023 16:55:17 +0100
Geert Uytterhoeven <geert+renesas@glider.be> 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.
> 
> Fixes: 275e4e2dc0411508 ("dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks good catch!

Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] dt-bindings: clk: vc5: Make SD/OE pin configuration properties not required

[Sean Anderson]

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:

Re: [PATCH] dt-bindings: clk: vc5: Make SD/OE pin configuration properties not required

[Luca Ceresoli]

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.
-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] dt-bindings: clk: vc5: Make SD/OE pin configuration properties not required

[Geert Uytterhoeven]

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.

> 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...

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] dt-bindings: clk: vc5: Make SD/OE pin configuration properties not required

[Sean Anderson]

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

Re: [PATCH] dt-bindings: clk: vc5: Make SD/OE pin configuration properties not required

[Stephen Boyd]

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.

Re: [PATCH] dt-bindings: clk: vc5: Make SD/OE pin configuration properties not required

[Luca Ceresoli]

Hello Stephen,

On Mon, 20 Mar 2023 14:27:56 -0700
Stephen Boyd <sboyd@kernel.org> wrote:

> 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.

IIRC Geert kind of accepted the idea that these properties should stay
required. Which is a bit annoying but it's the safest option, so unless
there are new complaints with solid use cases for making them optionalm,
I think it's OK to drop the patch.

Luca
-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] dt-bindings: clk: vc5: Make SD/OE pin configuration properties not required

[Rob Herring]

On Wed, Mar 22, 2023 at 3:39 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
>
> Hello Stephen,
>
> On Mon, 20 Mar 2023 14:27:56 -0700
> Stephen Boyd <sboyd@kernel.org> wrote:
>
> > 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.
>
> IIRC Geert kind of accepted the idea that these properties should stay
> required. Which is a bit annoying but it's the safest option, so unless
> there are new complaints with solid use cases for making them optionalm,
> I think it's OK to drop the patch.

The warnings related to this are now at the top of the list (by number
of occurrences):

     50 clock-generator@6a: 'idt,shutdown' is a required property
     50 clock-generator@6a: 'idt,output-enable-active' is a required property

IMO, if these properties haven't been needed for years, then they
obviously aren't really required.

Rob

Re: [PATCH] dt-bindings: clk: vc5: Make SD/OE pin configuration properties not required

[Luca Ceresoli]

Hello Sean, Geert,

On Tue, 10 Sep 2024 17:13:55 -0500
Rob Herring <robh+dt@kernel.org> wrote:

> On Wed, Mar 22, 2023 at 3:39 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> >
> > Hello Stephen,
> >
> > On Mon, 20 Mar 2023 14:27:56 -0700
> > Stephen Boyd <sboyd@kernel.org> wrote:
> >  
> > > 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.  
> >
> > IIRC Geert kind of accepted the idea that these properties should stay
> > required. Which is a bit annoying but it's the safest option, so unless
> > there are new complaints with solid use cases for making them optionalm,
> > I think it's OK to drop the patch.  
> 
> The warnings related to this are now at the top of the list (by number
> of occurrences):
> 
>      50 clock-generator@6a: 'idt,shutdown' is a required property
>      50 clock-generator@6a: 'idt,output-enable-active' is a required property
> 
> IMO, if these properties haven't been needed for years, then they
> obviously aren't really required.

I think Rob's point adds to Geert's observation that there are other
"idt,*" properties in the output nodes that may also be important to
have correctly set, and are optional.

So, Sean, I understand when you state it's safer to have these set.
However this is valid for lots of other optional properties in any
binding. Optional properties _can_ be set if that's important, just
it's not mandatory to set them in all cases.

As a matter of fact, we have been having for a long time some in-tree
device trees which don't set these properties, which I believe implies
it's OK for those cases to not set them, and to let them be set for the
device trees where it is important.

Finally, there is a maintenance/legacy issue: if we wanted to keep these
properties optional, who would chase all the boards defined in existing
device trees to discover the correct values?

Bottom line, my Reviewed-by tag is still valid.

What is your opinion given these last few discussion point Sean?

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] dt-bindings: clk: vc5: Make SD/OE pin configuration properties not required

[Sean Anderson]

On 9/13/24 11:07, Luca Ceresoli wrote:
> Hello Sean, Geert,
> 
> On Tue, 10 Sep 2024 17:13:55 -0500
> Rob Herring <robh+dt@kernel.org> wrote:
> 
>> On Wed, Mar 22, 2023 at 3:39 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
>> >
>> > Hello Stephen,
>> >
>> > On Mon, 20 Mar 2023 14:27:56 -0700
>> > Stephen Boyd <sboyd@kernel.org> wrote:
>> >  
>> > > 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.  
>> >
>> > IIRC Geert kind of accepted the idea that these properties should stay
>> > required. Which is a bit annoying but it's the safest option, so unless
>> > there are new complaints with solid use cases for making them optionalm,
>> > I think it's OK to drop the patch.  
>> 
>> The warnings related to this are now at the top of the list (by number
>> of occurrences):
>> 
>>      50 clock-generator@6a: 'idt,shutdown' is a required property
>>      50 clock-generator@6a: 'idt,output-enable-active' is a required property
>> 
>> IMO, if these properties haven't been needed for years, then they
>> obviously aren't really required.
> 
> I think Rob's point adds to Geert's observation that there are other
> "idt,*" properties in the output nodes that may also be important to
> have correctly set, and are optional.
> 
> So, Sean, I understand when you state it's safer to have these set.
> However this is valid for lots of other optional properties in any
> binding. Optional properties _can_ be set if that's important, just
> it's not mandatory to set them in all cases.
> 
> As a matter of fact, we have been having for a long time some in-tree
> device trees which don't set these properties, which I believe implies
> it's OK for those cases to not set them, and to let them be set for the
> device trees where it is important.
> 
> Finally, there is a maintenance/legacy issue: if we wanted to keep these
> properties optional, who would chase all the boards defined in existing
> device trees to discover the correct values?
> 
> Bottom line, my Reviewed-by tag is still valid.
> 
> What is your opinion given these last few discussion point Sean?

I am willing to send patches adding these properties for the appropriate
boards. There are only 6 in tree (all Renesas):

$ git grep -l idt,5p49 '**.dts*'
arch/arm64/boot/dts/renesas/beacon-renesom-baseboard.dtsi
arch/arm64/boot/dts/renesas/beacon-renesom-som.dtsi
arch/arm64/boot/dts/renesas/hihope-common.dtsi
arch/arm64/boot/dts/renesas/salvator-x.dtsi
arch/arm64/boot/dts/renesas/salvator-xs.dtsi
arch/arm64/boot/dts/renesas/ulcb.dtsi

I was able to find schematics for ULCB. Salvator-X seems to be gone from
Renesas's website (in favor of the -XS). I have requested access to the
-XS schematics.  The HiHope board doesn't seem to have schematics
anywhere I could find (which is pretty unusual for a reference
design...). The Beacon schematics are behind a support portal (or so I
assume).

That said, this info should be pretty easy to find for anyone with
physical access to a board. Just boot it up and probe the voltage on the
SD/OE pin. I've added some people who may have the hardware to CC.

--Sean

Re: [PATCH] dt-bindings: clk: vc5: Make SD/OE pin configuration properties not required

[Rob Herring]

On Fri, Sep 13, 2024 at 11:41 AM Sean Anderson <sean.anderson@seco.com> wrote:
>
> On 9/13/24 11:07, Luca Ceresoli wrote:
> > Hello Sean, Geert,
> >
> > On Tue, 10 Sep 2024 17:13:55 -0500
> > Rob Herring <robh+dt@kernel.org> wrote:
> >
> >> On Wed, Mar 22, 2023 at 3:39 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> >> >
> >> > Hello Stephen,
> >> >
> >> > On Mon, 20 Mar 2023 14:27:56 -0700
> >> > Stephen Boyd <sboyd@kernel.org> wrote:
> >> >
> >> > > 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.
> >> >
> >> > IIRC Geert kind of accepted the idea that these properties should stay
> >> > required. Which is a bit annoying but it's the safest option, so unless
> >> > there are new complaints with solid use cases for making them optionalm,
> >> > I think it's OK to drop the patch.
> >>
> >> The warnings related to this are now at the top of the list (by number
> >> of occurrences):
> >>
> >>      50 clock-generator@6a: 'idt,shutdown' is a required property
> >>      50 clock-generator@6a: 'idt,output-enable-active' is a required property
> >>
> >> IMO, if these properties haven't been needed for years, then they
> >> obviously aren't really required.
> >
> > I think Rob's point adds to Geert's observation that there are other
> > "idt,*" properties in the output nodes that may also be important to
> > have correctly set, and are optional.
> >
> > So, Sean, I understand when you state it's safer to have these set.
> > However this is valid for lots of other optional properties in any
> > binding. Optional properties _can_ be set if that's important, just
> > it's not mandatory to set them in all cases.
> >
> > As a matter of fact, we have been having for a long time some in-tree
> > device trees which don't set these properties, which I believe implies
> > it's OK for those cases to not set them, and to let them be set for the
> > device trees where it is important.
> >
> > Finally, there is a maintenance/legacy issue: if we wanted to keep these
> > properties optional, who would chase all the boards defined in existing
> > device trees to discover the correct values?
> >
> > Bottom line, my Reviewed-by tag is still valid.
> >
> > What is your opinion given these last few discussion point Sean?
>
> I am willing to send patches adding these properties for the appropriate
> boards. There are only 6 in tree (all Renesas):
>
> $ git grep -l idt,5p49 '**.dts*'
> arch/arm64/boot/dts/renesas/beacon-renesom-baseboard.dtsi
> arch/arm64/boot/dts/renesas/beacon-renesom-som.dtsi
> arch/arm64/boot/dts/renesas/hihope-common.dtsi
> arch/arm64/boot/dts/renesas/salvator-x.dtsi
> arch/arm64/boot/dts/renesas/salvator-xs.dtsi
> arch/arm64/boot/dts/renesas/ulcb.dtsi
>
> I was able to find schematics for ULCB. Salvator-X seems to be gone from
> Renesas's website (in favor of the -XS). I have requested access to the
> -XS schematics.  The HiHope board doesn't seem to have schematics
> anywhere I could find (which is pretty unusual for a reference
> design...). The Beacon schematics are behind a support portal (or so I
> assume).

That doesn't sound promising to me.

> That said, this info should be pretty easy to find for anyone with
> physical access to a board. Just boot it up and probe the voltage on the
> SD/OE pin. I've added some people who may have the hardware to CC.

By some definition of easy I guess...

I want the warning gone, so I'm going to apply this patch. When/if all
the cases have been fixed, I'll happily revert it.

Rob