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

[Sean Anderson <sean.anderson@seco.com>]

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