| Message ID | 20210629154740.3091884-1-sean.anderson@seco.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin | expand |
On 6/30/21 5:12 AM, Geert Uytterhoeven wrote: > 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. 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
Hi Sean, On Thu, Jul 1, 2021 at 5:52 PM Sean Anderson <sean.anderson@seco.com> wrote: > On 6/30/21 5:12 AM, Geert Uytterhoeven wrote: > > 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. > > 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. Sure, we can only make them required once all in-tree DTS files have been fixed. 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
Hi Geert, On 01/07/21 18:44, Geert Uytterhoeven wrote: [...] >>>> 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. > > Sure, we can only make them required once all in-tree DTS files have been > fixed. Good this is your opinion: as all of the vc5/6 instances in mainline Linux are on Renesas dts[i] files, so I guess we can count on you to fix them. :) -- Luca
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. + maintainers: - Luca Ceresoli <luca@lucaceresoli.net> @@ -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. + + idt,output-enable-active-high: + $ref: /schemas/types.yaml#/definitions/flag + description: | + This enables output when the SD/OE pin is high, and disables + output when the SD/OE pin is low. This corresponds to setting the + SP bit of the Primary Source and Shutdown Register. + + idt,output-enable-active-low: + $ref: /schemas/types.yaml#/definitions/flag + description: | + This disables output when the SD/OE pin is high, and enables + output when the SD/OE pin is low. This corresponds to clearing the + SP bit of the Primary Source and Shutdown Register. + patternProperties: "^OUT[1-4]$": type: object