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

[Geert Uytterhoeven <geert@linux-m68k.org>]

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