On 6/30/21 5:12 AM, Geert Uytterhoeven wrote:
Hi Luca,
On Wed, Jun 30, 2021 at 9:57 AM Luca Ceresoli <luca@xxxxxxxxxxxxxxxx> 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@xxxxxxxx>
>>> 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
