| Message ID | 68037ad181991fe0b792f6d003e3e9e538d5ffd7.1673452118.git.geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | dt-bindings: clk: vc5: Make SD/OE pin configuration properties not required | expand |
On 11/01/2023 16: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 > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
Hi Geert, On Wed, 11 Jan 2023 16:55:17 +0100 Geert Uytterhoeven <geert+renesas@glider.be> 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. > > Fixes: 275e4e2dc0411508 ("dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin") > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> Thanks good catch! Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
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. > Fixes: 275e4e2dc0411508 ("dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin") This was intentional... not a fix IMO. --Sean > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > Documentation/devicetree/bindings/clock/idt,versaclock5.yaml | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml > index 61b246cf5e72aa47..a5472bbfb8d1fdcc 100644 > --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml > +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml > @@ -125,8 +125,6 @@ required: > - compatible > - reg > - '#clock-cells' > - - idt,shutdown > - - idt,output-enable-active > > allOf: > - if:
Hi Sean, Geert, 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). To some extent, writing settings in an OTP is similar to producing a different chip where these values are hard-coded and not configured. I'm wondering whether Geert has a practical example of a situation where it is better to have these properties optional. Best regards.
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. > 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... Thanks! 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
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
Quoting Sean Anderson (2023-01-24 08:23:45) > 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: > > > >> 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. > Is there any resolution on this thread? I'm dropping this patch from my queue.
Hello Stephen, On Mon, 20 Mar 2023 14:27:56 -0700 Stephen Boyd <sboyd@kernel.org> wrote: > Quoting Sean Anderson (2023-01-24 08:23:45) > > 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: > > > > > >> 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. > > > > Is there any resolution on this thread? I'm dropping this patch from my > queue. IIRC Geert kind of accepted the idea that these properties should stay required. Which is a bit annoying but it's the safest option, so unless there are new complaints with solid use cases for making them optionalm, I think it's OK to drop the patch. Luca
diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml index 61b246cf5e72aa47..a5472bbfb8d1fdcc 100644 --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml @@ -125,8 +125,6 @@ required: - compatible - reg - '#clock-cells' - - idt,shutdown - - idt,output-enable-active allOf: - if:
"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. Fixes: 275e4e2dc0411508 ("dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin") Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- Documentation/devicetree/bindings/clock/idt,versaclock5.yaml | 2 -- 1 file changed, 2 deletions(-)