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

[Luca Ceresoli <luca@lucaceresoli.net>]

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