[PATCH v3 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin

[PATCH v3 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin

[Sean Anderson]

These properties allow configuring the SD/OE pin as described in the
datasheet.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
Acked-by: Rob Herring <robh@kernel.org>
---

Changes in v3:
- Add idt,disable-shutdown and idt,output-enable-active-low to allow for
  a default of not changing the SP/SH bits at all.

Changes in v2:
- Rename idt,sd-active-high to idt,output-enable-active-high
- Add idt,enable-shutdown

 .../bindings/clock/idt,versaclock5.yaml       | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
index 28675b0b80f1..51f0f78cc3f4 100644
--- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
+++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
@@ -30,6 +30,22 @@ description: |
     3 -- OUT3
     4 -- OUT4
 
+  The idt,(en|dis)able-shutdown and idt,output-enable-active-(high|low)
+  properties control the SH (en_global_shutdown) and SP bits of the
+  Primary Source and Shutdown Register, respectively. Their behavior is
+  summarized by the following table:
+
+  SH SP Output when the SD/OE pin is Low/High
+  == == =====================================
+   0  0 Active/Inactive
+   0  1 Inactive/Active
+   1  0 Active/Shutdown
+   1  1 Inactive/Shutdown
+
+  If no properties related to these bits are specified, then they will
+  be left in their default state. This may be useful if the SH and SP
+  bits are set to a default value using the OTP memory.
+
 maintainers:
   - Luca Ceresoli <luca@lucaceresoli.net>
 
@@ -64,6 +80,34 @@ properties:
     maximum: 22760
     description: Optional load capacitor for XTAL1 and XTAL2
 
+  idt,enable-shutdown:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description: |
+      Enable the shutdown function when the SD/OE pin is high. This
+      corresponds to setting the SH bit of the Primary Source and
+      Shutdown Register.
+
+  idt,disable-shutdown:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description: |
+      Disable the shutdown function for the SD/OE pin. This corresponds
+      to clearing the SH bit of the Primary Source and Shutdown
+      Register.
+
+  idt,output-enable-active-high:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description: |
+      This enables output when the SD/OE pin is high, and disables
+      output when the SD/OE pin is low. This corresponds to setting the
+      SP bit of the Primary Source and Shutdown Register.
+
+  idt,output-enable-active-low:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description: |
+      This disables output when the SD/OE pin is high, and enables
+      output when the SD/OE pin is low. This corresponds to clearing the
+      SP bit of the Primary Source and Shutdown Register.
+
 patternProperties:
   "^OUT[1-4]$":
     type: object
-- 
2.25.1

Re: [PATCH v3 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin

[Stephen Boyd]

Quoting Sean Anderson (2021-06-29 08:47:39)
> These properties allow configuring the SD/OE pin as described in the
> datasheet.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> Acked-by: Rob Herring <robh@kernel.org>
> ---

I dropped v2 from clk-next and this will be punted to the next release.

Re: [PATCH v3 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin

[Luca Ceresoli]

Hi Sean,

On 29/06/21 17:47, Sean Anderson wrote:
> These properties allow configuring the SD/OE pin as described in the
> datasheet.

*Many* thanks for addressing this issue so quickly!

> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> Acked-by: Rob Herring <robh@kernel.org>

I don't think Rob's ack should be present, he hasn't approved _this_
version of the patch.

> ---
> 
> Changes in v3:
> - Add idt,disable-shutdown and idt,output-enable-active-low to allow for
>   a default of not changing the SP/SH bits at all.
> 
> Changes in v2:
> - Rename idt,sd-active-high to idt,output-enable-active-high
> - Add idt,enable-shutdown
> 
>  .../bindings/clock/idt,versaclock5.yaml       | 44 +++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> index 28675b0b80f1..51f0f78cc3f4 100644
> --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> @@ -30,6 +30,22 @@ description: |
>      3 -- OUT3
>      4 -- OUT4
>  
> +  The idt,(en|dis)able-shutdown and idt,output-enable-active-(high|low)
> +  properties control the SH (en_global_shutdown) and SP bits of the
> +  Primary Source and Shutdown Register, respectively. Their behavior is
> +  summarized by the following table:
> +
> +  SH SP Output when the SD/OE pin is Low/High
> +  == == =====================================
> +   0  0 Active/Inactive
> +   0  1 Inactive/Active
> +   1  0 Active/Shutdown
> +   1  1 Inactive/Shutdown
> +
> +  If no properties related to these bits are specified, then they will
> +  be left in their default state. This may be useful if the SH and SP
> +  bits are set to a default value using the OTP memory.

This paragraph looks more an implementation description than a hardware
description.

I suggest something like (possibly better rephrased):

It is recommended to specify the two properties that describe the
hardware. The lack of them leaves the value unspecified and thus opens
to the risk of future incompatibilities, depending on implementation
details.

> @@ -64,6 +80,34 @@ properties:
>      maximum: 22760
>      description: Optional load capacitor for XTAL1 and XTAL2
>  
> +  idt,enable-shutdown:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description: |
> +      Enable the shutdown function when the SD/OE pin is high. This
> +      corresponds to setting the SH bit of the Primary Source and
> +      Shutdown Register.
> +
> +  idt,disable-shutdown:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description: |
> +      Disable the shutdown function for the SD/OE pin. This corresponds
> +      to clearing the SH bit of the Primary Source and Shutdown
> +      Register.

Saying "Disable the shutdown function" leaves a hole, it is not telling
what gets enabled. I'd rephrase using positive logic:

  Enable the OE (output enable) function for the SD/OE pin. This...

But there are too many "enable" words in it now, it's confusing, so why not:

  Choose the OE (output enable) function for the SD/OE pin. This...

And change correspondingly the idt,enable-shutdown description:
s/^Enable/Choose/.

Also it would be nice to declare in DT that the two flags are mutually
exclusive (same for idt,output-enable-active-*).

-- 
Luca

Re: [PATCH v3 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin

[Sean Anderson]

On 6/29/21 5:23 PM, Luca Ceresoli wrote:
> Hi Sean,
>
> On 29/06/21 17:47, Sean Anderson wrote:
>> These properties allow configuring the SD/OE pin as described in the
>> datasheet.
>
> *Many* thanks for addressing this issue so quickly!
>
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> Acked-by: Rob Herring <robh@kernel.org>
>
> I don't think Rob's ack should be present, he hasn't approved _this_
> version of the patch.

Sorry, I was unsure whether I should keep it or not.

>
>> ---
>>
>> Changes in v3:
>> - Add idt,disable-shutdown and idt,output-enable-active-low to allow for
>>   a default of not changing the SP/SH bits at all.
>>
>> Changes in v2:
>> - Rename idt,sd-active-high to idt,output-enable-active-high
>> - Add idt,enable-shutdown
>>
>>  .../bindings/clock/idt,versaclock5.yaml       | 44 +++++++++++++++++++
>>  1 file changed, 44 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
>> index 28675b0b80f1..51f0f78cc3f4 100644
>> --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
>> +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
>> @@ -30,6 +30,22 @@ description: |
>>      3 -- OUT3
>>      4 -- OUT4
>>
>> +  The idt,(en|dis)able-shutdown and idt,output-enable-active-(high|low)
>> +  properties control the SH (en_global_shutdown) and SP bits of the
>> +  Primary Source and Shutdown Register, respectively. Their behavior is
>> +  summarized by the following table:
>> +
>> +  SH SP Output when the SD/OE pin is Low/High
>> +  == == =====================================
>> +   0  0 Active/Inactive
>> +   0  1 Inactive/Active
>> +   1  0 Active/Shutdown
>> +   1  1 Inactive/Shutdown
>> +
>> +  If no properties related to these bits are specified, then they will
>> +  be left in their default state. This may be useful if the SH and SP
>> +  bits are set to a default value using the OTP memory.
>
> This paragraph looks more an implementation description than a hardware
> description.

It of course *is* an implementation description. As Geert found out, it
is important to keep the defaults if none of these properties are
specified.

> I suggest something like (possibly better rephrased):
>
> It is recommended to specify the two properties that describe the
> hardware. The lack of them leaves the value unspecified and thus opens
> to the risk of future incompatibilities, depending on implementation
> details.

Ok, so if I understand correctly, you would like to deprecate existing
bindings which do not specify any of these properties.

>
>> @@ -64,6 +80,34 @@ properties:
>>      maximum: 22760
>>      description: Optional load capacitor for XTAL1 and XTAL2
>>
>> +  idt,enable-shutdown:
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +    description: |
>> +      Enable the shutdown function when the SD/OE pin is high. This
>> +      corresponds to setting the SH bit of the Primary Source and
>> +      Shutdown Register.
>> +
>> +  idt,disable-shutdown:
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +    description: |
>> +      Disable the shutdown function for the SD/OE pin. This corresponds
>> +      to clearing the SH bit of the Primary Source and Shutdown
>> +      Register.
>
> Saying "Disable the shutdown function" leaves a hole, it is not telling
> what gets enabled. I'd rephrase using positive logic:
>
>    Enable the OE (output enable) function for the SD/OE pin. This...
>
> But there are too many "enable" words in it now, it's confusing, so why not:
>
>    Choose the OE (output enable) function for the SD/OE pin. This...

The issue here is that the OE function is in some sense always enabled.
So perhaps a better wording would be

	Disable the shutdown functionality. The chip will never be
	shut down based on the value of the SD/OE pin.

And for enable-shutdown

	Enable the shutdown functionality. The chip will be shut down if
	the SD/OE pin is driven high.

> And change correspondingly the idt,enable-shutdown description:
> s/^Enable/Choose/.
>
> Also it would be nice to declare in DT that the two flags are mutually
> exclusive (same for idt,output-enable-active-*).

Ok, will fix in v4.

--Sean

Re: [PATCH v3 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin

[Luca Ceresoli]

Hi,

On 29/06/21 23:41, Sean Anderson wrote:
> 
> 
> On 6/29/21 5:23 PM, Luca Ceresoli wrote:
>> Hi Sean,
>>
>> On 29/06/21 17:47, Sean Anderson wrote:
>>> These properties allow configuring the SD/OE pin as described in the
>>> datasheet.
>>
>> *Many* thanks for addressing this issue so quickly!
>>
>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>>> Acked-by: Rob Herring <robh@kernel.org>
>>
>> I don't think Rob's ack should be present, he hasn't approved _this_
>> version of the patch.
> 
> Sorry, I was unsure whether I should keep it or not.

It's OK if the patch is unchanged or has only changes that are
minimal/irrelevant.

>>> ---
>>>
>>> Changes in v3:
>>> - Add idt,disable-shutdown and idt,output-enable-active-low to allow for
>>>   a default of not changing the SP/SH bits at all.
>>>
>>> Changes in v2:
>>> - Rename idt,sd-active-high to idt,output-enable-active-high
>>> - Add idt,enable-shutdown
>>>
>>>  .../bindings/clock/idt,versaclock5.yaml       | 44 +++++++++++++++++++
>>>  1 file changed, 44 insertions(+)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
>>> b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
>>> index 28675b0b80f1..51f0f78cc3f4 100644
>>> --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
>>> +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
>>> @@ -30,6 +30,22 @@ description: |
>>>      3 -- OUT3
>>>      4 -- OUT4
>>>
>>> +  The idt,(en|dis)able-shutdown and idt,output-enable-active-(high|low)
>>> +  properties control the SH (en_global_shutdown) and SP bits of the
>>> +  Primary Source and Shutdown Register, respectively. Their behavior is
>>> +  summarized by the following table:
>>> +
>>> +  SH SP Output when the SD/OE pin is Low/High
>>> +  == == =====================================
>>> +   0  0 Active/Inactive
>>> +   0  1 Inactive/Active
>>> +   1  0 Active/Shutdown
>>> +   1  1 Inactive/Shutdown
>>> +
>>> +  If no properties related to these bits are specified, then they will
>>> +  be left in their default state. This may be useful if the SH and SP
>>> +  bits are set to a default value using the OTP memory.
>>
>> This paragraph looks more an implementation description than a hardware
>> description.
> 
> It of course *is* an implementation description. As Geert found out, it
> is important to keep the defaults if none of these properties are
> specified.

DT should describe hardware, not implementation. The difference is
subtle at times, but it is important. Other OSes, bootloaders,
firmwares, whatever can have a totally different implementation but use
the same DT.

>> I suggest something like (possibly better rephrased):
>>
>> It is recommended to specify the two properties that describe the
>> hardware. The lack of them leaves the value unspecified and thus opens
>> to the risk of future incompatibilities, depending on implementation
>> details.
> 
> Ok, so if I understand correctly, you would like to deprecate existing
> bindings which do not specify any of these properties.

Well, the first goal of my rephrasing was to change from "If no
properties are specified, then they will be left in their default state"
(telling what _your_ _Linux_ implementation _does_) to "if you don't
tell how your HW is done, the implementation is not in a position to do
the right thing" (telling what DT must _describe_ to _any_
implementation for it to work properly). I hope this clarifies the idea.
Again, it's a subtle but very important difference.

While rephrasing I also thought I would be more explicit than "if
(unspecified) {can't do the right thig}" and go direct: "you should
specify either of them". I would like to say "must" in place of "should"
but we must take care of existing DTs.

Perhaps these properties might be made mandatory later, after upgrading
all DTs (at least those in mainline Linux). and a grace period.

>>> @@ -64,6 +80,34 @@ properties:
>>>      maximum: 22760
>>>      description: Optional load capacitor for XTAL1 and XTAL2
>>>
>>> +  idt,enable-shutdown:
>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>> +    description: |
>>> +      Enable the shutdown function when the SD/OE pin is high. This
>>> +      corresponds to setting the SH bit of the Primary Source and
>>> +      Shutdown Register.
>>> +
>>> +  idt,disable-shutdown:
>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>> +    description: |
>>> +      Disable the shutdown function for the SD/OE pin. This corresponds
>>> +      to clearing the SH bit of the Primary Source and Shutdown
>>> +      Register.
>>
>> Saying "Disable the shutdown function" leaves a hole, it is not telling
>> what gets enabled. I'd rephrase using positive logic:
>>
>>    Enable the OE (output enable) function for the SD/OE pin. This...
>>
>> But there are too many "enable" words in it now, it's confusing, so
>> why not:
>>
>>    Choose the OE (output enable) function for the SD/OE pin. This...
> 
> The issue here is that the OE function is in some sense always enabled.
> So perhaps a better wording would be
> 
>     Disable the shutdown functionality. The chip will never be
>     shut down based on the value of the SD/OE pin.
> 
> And for enable-shutdown
> 
>     Enable the shutdown functionality. The chip will be shut down if
>     the SD/OE pin is driven high.

This used to be my understanding too. However I recently got this from
Renesas support and got confused again: "SH=1 means that you want the
ShutDown function.  ShutDown fixes the polarity to active low so the SP
bit becomes don’t care.".

However looking at table 5 in the 5P49V6901 datasheet and other docs it
seems you are right, and SP is don't care if SH=1 AND SD/OE pin is low.

Forget my comment, this latest definition from you seems very good, but
please keep also the "This corresponds to..." sentences, they remove any
potential bad misunderstanding.

-- 
Luca

Re: [PATCH v3 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin

[Geert Uytterhoeven]

Hi Luca,

On Wed, Jun 30, 2021 at 9:57 AM Luca Ceresoli <luca@lucaceresoli.net> wrote:
> On 29/06/21 23:41, Sean Anderson wrote:
> > On 6/29/21 5:23 PM, Luca Ceresoli wrote:
> >> On 29/06/21 17:47, Sean Anderson wrote:
> >>> These properties allow configuring the SD/OE pin as described in the
> >>> datasheet.
> >>
> >> *Many* thanks for addressing this issue so quickly!
> >>
> >>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>

> >>> a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> >>> b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> >>> index 28675b0b80f1..51f0f78cc3f4 100644
> >>> --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> >>> +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> >>> @@ -30,6 +30,22 @@ description: |
> >>>      3 -- OUT3
> >>>      4 -- OUT4
> >>>
> >>> +  The idt,(en|dis)able-shutdown and idt,output-enable-active-(high|low)
> >>> +  properties control the SH (en_global_shutdown) and SP bits of the
> >>> +  Primary Source and Shutdown Register, respectively. Their behavior is
> >>> +  summarized by the following table:
> >>> +
> >>> +  SH SP Output when the SD/OE pin is Low/High
> >>> +  == == =====================================
> >>> +   0  0 Active/Inactive
> >>> +   0  1 Inactive/Active
> >>> +   1  0 Active/Shutdown
> >>> +   1  1 Inactive/Shutdown
> >>> +
> >>> +  If no properties related to these bits are specified, then they will
> >>> +  be left in their default state. This may be useful if the SH and SP
> >>> +  bits are set to a default value using the OTP memory.
> >>
> >> This paragraph looks more an implementation description than a hardware
> >> description.
> >
> > It of course *is* an implementation description. As Geert found out, it
> > is important to keep the defaults if none of these properties are
> > specified.
>
> DT should describe hardware, not implementation. The difference is
> subtle at times, but it is important. Other OSes, bootloaders,
> firmwares, whatever can have a totally different implementation but use
> the same DT.

In general, it's best for a driver not to rely on any preprogramming
done by e.g. the bootloader before.

The concept of "One-Time Programming (OTP) bits" adds yet another
dimension to the already complicated boot chain of dependencies.
But due to the one-time feature I consider that more stable than
other firmware, which can be upgraded, causing changed behavior,
unlike OTP bits.

> Perhaps these properties might be made mandatory later, after upgrading
> all DTs (at least those in mainline Linux). and a grace period.

Yes, they should be marked as required.
But the driver can keep on not touching the bits if none of
the new properties is specified.

BTW, I didn't check the datasheet, but is there a way to read the OTP
bits from software?  If yes, the driver could use these values if the
new properties are not present.  That would make the system more
robust, as it would reset the values (if ever changed) to a sane
state in case of a soft reboot.

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 v3 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin

[Sean Anderson]

On 6/30/21 5:12 AM, Geert Uytterhoeven wrote:
> Hi Luca,
>
> On Wed, Jun 30, 2021 at 9:57 AM Luca Ceresoli <luca@lucaceresoli.net> wrote:
>> On 29/06/21 23:41, Sean Anderson wrote:
>> > On 6/29/21 5:23 PM, Luca Ceresoli wrote:
>> >> On 29/06/21 17:47, Sean Anderson wrote:
>> >>> These properties allow configuring the SD/OE pin as described in the
>> >>> datasheet.
>> >>
>> >> *Many* thanks for addressing this issue so quickly!
>> >>
>> >>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>
>> >>> a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
>> >>> b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
>> >>> index 28675b0b80f1..51f0f78cc3f4 100644
>> >>> --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
>> >>> +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
>> >>> @@ -30,6 +30,22 @@ description: |
>> >>>      3 -- OUT3
>> >>>      4 -- OUT4
>> >>>
>> >>> +  The idt,(en|dis)able-shutdown and idt,output-enable-active-(high|low)
>> >>> +  properties control the SH (en_global_shutdown) and SP bits of the
>> >>> +  Primary Source and Shutdown Register, respectively. Their behavior is
>> >>> +  summarized by the following table:
>> >>> +
>> >>> +  SH SP Output when the SD/OE pin is Low/High
>> >>> +  == == =====================================
>> >>> +   0  0 Active/Inactive
>> >>> +   0  1 Inactive/Active
>> >>> +   1  0 Active/Shutdown
>> >>> +   1  1 Inactive/Shutdown
>> >>> +
>> >>> +  If no properties related to these bits are specified, then they will
>> >>> +  be left in their default state. This may be useful if the SH and SP
>> >>> +  bits are set to a default value using the OTP memory.
>> >>
>> >> This paragraph looks more an implementation description than a hardware
>> >> description.
>> >
>> > It of course *is* an implementation description. As Geert found out, it
>> > is important to keep the defaults if none of these properties are
>> > specified.
>>
>> DT should describe hardware, not implementation. The difference is
>> subtle at times, but it is important. Other OSes, bootloaders,
>> firmwares, whatever can have a totally different implementation but use
>> the same DT.
>
> In general, it's best for a driver not to rely on any preprogramming
> done by e.g. the bootloader before.

This is part of the reason I wanted to add these properties in the first
place. I'm working on a board where one version has had the OTP
programmed, and one version has not. But of course, if we set these bits
in software then I do not have to worry about whether the OTP has set up
something sane.

>
> The concept of "One-Time Programming (OTP) bits" adds yet another
> dimension to the already complicated boot chain of dependencies.
> But due to the one-time feature I consider that more stable than
> other firmware, which can be upgraded, causing changed behavior,
> unlike OTP bits.
>
>> Perhaps these properties might be made mandatory later, after upgrading
>> all DTs (at least those in mainline Linux). and a grace period.
>
> Yes, they should be marked as required.

I don't think I can do that without going through all existing users and
defining these properties for them. Otherwise, dt_bindings_check will
complain. I believe (but please correct me if I'm wrong) that patches
are not to introduce new warnings.

However, setting these propreties is not possible for me to do; I would
need someone familiar with their board to determine how the SD/OE pin is
used, and what the correct setting is.

> But the driver can keep on not touching the bits if none of
> the new properties is specified.
>
> BTW, I didn't check the datasheet, but is there a way to read the OTP
> bits from software?  If yes, the driver could use these values if the
> new properties are not present.  That would make the system more
> robust, as it would reset the values (if ever changed) to a sane
> state in case of a soft reboot.

AIUI the OTP bits are automatically loaded into RAM when the device
powers up. So I don't think we need to do anything other than leave
these bits alone if we don't have any related properties.

--Sean

Re: [PATCH v3 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin

[Geert Uytterhoeven]

Hi Sean,

On Thu, Jul 1, 2021 at 5:52 PM Sean Anderson <sean.anderson@seco.com> wrote:
> On 6/30/21 5:12 AM, Geert Uytterhoeven wrote:
> > On Wed, Jun 30, 2021 at 9:57 AM Luca Ceresoli <luca@lucaceresoli.net> wrote:
> >> On 29/06/21 23:41, Sean Anderson wrote:
> >> > On 6/29/21 5:23 PM, Luca Ceresoli wrote:
> >> >> On 29/06/21 17:47, Sean Anderson wrote:
> >> >>> These properties allow configuring the SD/OE pin as described in the
> >> >>> datasheet.
> >> >>
> >> >> *Many* thanks for addressing this issue so quickly!
> >> >>
> >> >>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> >
> >> >>> a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> >> >>> b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> >> >>> index 28675b0b80f1..51f0f78cc3f4 100644
> >> >>> --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> >> >>> +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> >> >>> @@ -30,6 +30,22 @@ description: |
> >> >>>      3 -- OUT3
> >> >>>      4 -- OUT4
> >> >>>
> >> >>> +  The idt,(en|dis)able-shutdown and idt,output-enable-active-(high|low)
> >> >>> +  properties control the SH (en_global_shutdown) and SP bits of the
> >> >>> +  Primary Source and Shutdown Register, respectively. Their behavior is
> >> >>> +  summarized by the following table:
> >> >>> +
> >> >>> +  SH SP Output when the SD/OE pin is Low/High
> >> >>> +  == == =====================================
> >> >>> +   0  0 Active/Inactive
> >> >>> +   0  1 Inactive/Active
> >> >>> +   1  0 Active/Shutdown
> >> >>> +   1  1 Inactive/Shutdown
> >> >>> +
> >> >>> +  If no properties related to these bits are specified, then they will
> >> >>> +  be left in their default state. This may be useful if the SH and SP
> >> >>> +  bits are set to a default value using the OTP memory.
> >> >>
> >> >> This paragraph looks more an implementation description than a hardware
> >> >> description.
> >> >
> >> > It of course *is* an implementation description. As Geert found out, it
> >> > is important to keep the defaults if none of these properties are
> >> > specified.
> >>
> >> DT should describe hardware, not implementation. The difference is
> >> subtle at times, but it is important. Other OSes, bootloaders,
> >> firmwares, whatever can have a totally different implementation but use
> >> the same DT.
> >
> > In general, it's best for a driver not to rely on any preprogramming
> > done by e.g. the bootloader before.
>
> This is part of the reason I wanted to add these properties in the first
> place. I'm working on a board where one version has had the OTP
> programmed, and one version has not. But of course, if we set these bits
> in software then I do not have to worry about whether the OTP has set up
> something sane.
>
> >
> > The concept of "One-Time Programming (OTP) bits" adds yet another
> > dimension to the already complicated boot chain of dependencies.
> > But due to the one-time feature I consider that more stable than
> > other firmware, which can be upgraded, causing changed behavior,
> > unlike OTP bits.
> >
> >> Perhaps these properties might be made mandatory later, after upgrading
> >> all DTs (at least those in mainline Linux). and a grace period.
> >
> > Yes, they should be marked as required.
>
> I don't think I can do that without going through all existing users and
> defining these properties for them. Otherwise, dt_bindings_check will
> complain. I believe (but please correct me if I'm wrong) that patches
> are not to introduce new warnings.
>
> However, setting these propreties is not possible for me to do; I would
> need someone familiar with their board to determine how the SD/OE pin is
> used, and what the correct setting is.

Sure, we can only make them required once all in-tree DTS files have been
fixed.

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 v3 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin

[Luca Ceresoli]

Hi Geert,

On 01/07/21 18:44, Geert Uytterhoeven wrote:
[...]
>>>> Perhaps these properties might be made mandatory later, after upgrading
>>>> all DTs (at least those in mainline Linux). and a grace period.
>>>
>>> Yes, they should be marked as required.
>>
>> I don't think I can do that without going through all existing users and
>> defining these properties for them. Otherwise, dt_bindings_check will
>> complain. I believe (but please correct me if I'm wrong) that patches
>> are not to introduce new warnings.
>>
>> However, setting these propreties is not possible for me to do; I would
>> need someone familiar with their board to determine how the SD/OE pin is
>> used, and what the correct setting is.
> 
> Sure, we can only make them required once all in-tree DTS files have been
> fixed.

Good this is your opinion: as all of the vc5/6 instances in mainline
Linux are on Renesas dts[i] files, so I guess we can count on you to fix
them. :)

-- 
Luca