* [PATCH v3 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin @ 2021-06-29 15:47 Sean Anderson 2021-06-29 20:29 ` Stephen Boyd 2021-06-29 21:23 ` Luca Ceresoli 0 siblings, 2 replies; 9+ messages in thread From: Sean Anderson @ 2021-06-29 15:47 UTC (permalink / raw) To: linux-clk, Luca Ceresoli Cc: Geert Uytterhoeven, Adam Ford, Stephen Boyd, Michael Turquette, Sean Anderson, Rob Herring, devicetree These properties allow configuring the SD/OE pin as described in the datasheet. Signed-off-by: Sean Anderson <sean.anderson@seco.com> Acked-by: Rob Herring <robh@kernel.org> --- Changes in v3: - Add idt,disable-shutdown and idt,output-enable-active-low to allow for a default of not changing the SP/SH bits at all. Changes in v2: - Rename idt,sd-active-high to idt,output-enable-active-high - Add idt,enable-shutdown .../bindings/clock/idt,versaclock5.yaml | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) 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 -- 2.25.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin 2021-06-29 15:47 [PATCH v3 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin Sean Anderson @ 2021-06-29 20:29 ` Stephen Boyd 2021-06-29 21:23 ` Luca Ceresoli 1 sibling, 0 replies; 9+ messages in thread From: Stephen Boyd @ 2021-06-29 20:29 UTC (permalink / raw) To: Luca Ceresoli, Sean Anderson, linux-clk Cc: Geert Uytterhoeven, Adam Ford, Michael Turquette, Sean Anderson, Rob Herring, devicetree Quoting Sean Anderson (2021-06-29 08:47:39) > These properties allow configuring the SD/OE pin as described in the > datasheet. > > Signed-off-by: Sean Anderson <sean.anderson@seco.com> > Acked-by: Rob Herring <robh@kernel.org> > --- I dropped v2 from clk-next and this will be punted to the next release. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin 2021-06-29 15:47 [PATCH v3 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin Sean Anderson 2021-06-29 20:29 ` Stephen Boyd @ 2021-06-29 21:23 ` Luca Ceresoli 2021-06-29 21:41 ` Sean Anderson 1 sibling, 1 reply; 9+ messages in thread From: Luca Ceresoli @ 2021-06-29 21:23 UTC (permalink / raw) To: Sean Anderson, linux-clk Cc: Geert Uytterhoeven, Adam Ford, Stephen Boyd, Michael Turquette, Rob Herring, devicetree Hi Sean, 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> > Acked-by: Rob Herring <robh@kernel.org> I don't think Rob's ack should be present, he hasn't approved _this_ version of the patch. > --- > > Changes in v3: > - Add idt,disable-shutdown and idt,output-enable-active-low to allow for > a default of not changing the SP/SH bits at all. > > Changes in v2: > - Rename idt,sd-active-high to idt,output-enable-active-high > - Add idt,enable-shutdown > > .../bindings/clock/idt,versaclock5.yaml | 44 +++++++++++++++++++ > 1 file changed, 44 insertions(+) > > 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. This paragraph looks more an implementation description than a hardware description. I suggest something like (possibly better rephrased): It is recommended to specify the two properties that describe the hardware. The lack of them leaves the value unspecified and thus opens to the risk of future incompatibilities, depending on implementation details. > @@ -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. Saying "Disable the shutdown function" leaves a hole, it is not telling what gets enabled. I'd rephrase using positive logic: Enable the OE (output enable) function for the SD/OE pin. This... But there are too many "enable" words in it now, it's confusing, so why not: Choose the OE (output enable) function for the SD/OE pin. This... And change correspondingly the idt,enable-shutdown description: s/^Enable/Choose/. Also it would be nice to declare in DT that the two flags are mutually exclusive (same for idt,output-enable-active-*). -- Luca ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin 2021-06-29 21:23 ` Luca Ceresoli @ 2021-06-29 21:41 ` Sean Anderson 2021-06-30 7:57 ` Luca Ceresoli 0 siblings, 1 reply; 9+ messages in thread From: Sean Anderson @ 2021-06-29 21:41 UTC (permalink / raw) To: Luca Ceresoli, linux-clk Cc: Geert Uytterhoeven, Adam Ford, Stephen Boyd, Michael Turquette, Rob Herring, devicetree On 6/29/21 5:23 PM, Luca Ceresoli wrote: > Hi Sean, > > 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> >> Acked-by: Rob Herring <robh@kernel.org> > > I don't think Rob's ack should be present, he hasn't approved _this_ > version of the patch. Sorry, I was unsure whether I should keep it or not. > >> --- >> >> Changes in v3: >> - Add idt,disable-shutdown and idt,output-enable-active-low to allow for >> a default of not changing the SP/SH bits at all. >> >> Changes in v2: >> - Rename idt,sd-active-high to idt,output-enable-active-high >> - Add idt,enable-shutdown >> >> .../bindings/clock/idt,versaclock5.yaml | 44 +++++++++++++++++++ >> 1 file changed, 44 insertions(+) >> >> 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. > > 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. > I suggest something like (possibly better rephrased): > > It is recommended to specify the two properties that describe the > hardware. The lack of them leaves the value unspecified and thus opens > to the risk of future incompatibilities, depending on implementation > details. Ok, so if I understand correctly, you would like to deprecate existing bindings which do not specify any of these properties. > >> @@ -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. > > Saying "Disable the shutdown function" leaves a hole, it is not telling > what gets enabled. I'd rephrase using positive logic: > > Enable the OE (output enable) function for the SD/OE pin. This... > > But there are too many "enable" words in it now, it's confusing, so why not: > > Choose the OE (output enable) function for the SD/OE pin. This... The issue here is that the OE function is in some sense always enabled. So perhaps a better wording would be Disable the shutdown functionality. The chip will never be shut down based on the value of the SD/OE pin. And for enable-shutdown Enable the shutdown functionality. The chip will be shut down if the SD/OE pin is driven high. > And change correspondingly the idt,enable-shutdown description: > s/^Enable/Choose/. > > Also it would be nice to declare in DT that the two flags are mutually > exclusive (same for idt,output-enable-active-*). Ok, will fix in v4. --Sean ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin 2021-06-29 21:41 ` Sean Anderson @ 2021-06-30 7:57 ` Luca Ceresoli 2021-06-30 9:12 ` Geert Uytterhoeven 0 siblings, 1 reply; 9+ messages in thread From: Luca Ceresoli @ 2021-06-30 7:57 UTC (permalink / raw) To: Sean Anderson, linux-clk Cc: Geert Uytterhoeven, Adam Ford, Stephen Boyd, Michael Turquette, Rob Herring, devicetree Hi, On 29/06/21 23:41, Sean Anderson wrote: > > > On 6/29/21 5:23 PM, Luca Ceresoli wrote: >> Hi Sean, >> >> 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> >>> Acked-by: Rob Herring <robh@kernel.org> >> >> I don't think Rob's ack should be present, he hasn't approved _this_ >> version of the patch. > > Sorry, I was unsure whether I should keep it or not. It's OK if the patch is unchanged or has only changes that are minimal/irrelevant. >>> --- >>> >>> Changes in v3: >>> - Add idt,disable-shutdown and idt,output-enable-active-low to allow for >>> a default of not changing the SP/SH bits at all. >>> >>> Changes in v2: >>> - Rename idt,sd-active-high to idt,output-enable-active-high >>> - Add idt,enable-shutdown >>> >>> .../bindings/clock/idt,versaclock5.yaml | 44 +++++++++++++++++++ >>> 1 file changed, 44 insertions(+) >>> >>> 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. >> >> 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. >> I suggest something like (possibly better rephrased): >> >> It is recommended to specify the two properties that describe the >> hardware. The lack of them leaves the value unspecified and thus opens >> to the risk of future incompatibilities, depending on implementation >> details. > > Ok, so if I understand correctly, you would like to deprecate existing > bindings which do not specify any of these properties. Well, the first goal of my rephrasing was to change from "If no properties are specified, then they will be left in their default state" (telling what _your_ _Linux_ implementation _does_) to "if you don't tell how your HW is done, the implementation is not in a position to do the right thing" (telling what DT must _describe_ to _any_ implementation for it to work properly). I hope this clarifies the idea. Again, it's a subtle but very important difference. While rephrasing I also thought I would be more explicit than "if (unspecified) {can't do the right thig}" and go direct: "you should specify either of them". I would like to say "must" in place of "should" but we must take care of existing DTs. Perhaps these properties might be made mandatory later, after upgrading all DTs (at least those in mainline Linux). and a grace period. >>> @@ -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. >> >> Saying "Disable the shutdown function" leaves a hole, it is not telling >> what gets enabled. I'd rephrase using positive logic: >> >> Enable the OE (output enable) function for the SD/OE pin. This... >> >> But there are too many "enable" words in it now, it's confusing, so >> why not: >> >> Choose the OE (output enable) function for the SD/OE pin. This... > > The issue here is that the OE function is in some sense always enabled. > So perhaps a better wording would be > > Disable the shutdown functionality. The chip will never be > shut down based on the value of the SD/OE pin. > > And for enable-shutdown > > Enable the shutdown functionality. The chip will be shut down if > the SD/OE pin is driven high. This used to be my understanding too. However I recently got this from Renesas support and got confused again: "SH=1 means that you want the ShutDown function. ShutDown fixes the polarity to active low so the SP bit becomes don’t care.". However looking at table 5 in the 5P49V6901 datasheet and other docs it seems you are right, and SP is don't care if SH=1 AND SD/OE pin is low. Forget my comment, this latest definition from you seems very good, but please keep also the "This corresponds to..." sentences, they remove any potential bad misunderstanding. -- Luca ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin 2021-06-30 7:57 ` Luca Ceresoli @ 2021-06-30 9:12 ` Geert Uytterhoeven 2021-07-01 15:51 ` Sean Anderson 0 siblings, 1 reply; 9+ messages in thread From: Geert Uytterhoeven @ 2021-06-30 9:12 UTC (permalink / raw) To: Luca Ceresoli Cc: Sean Anderson, linux-clk, Adam Ford, Stephen Boyd, Michael Turquette, Rob Herring, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin 2021-06-30 9:12 ` Geert Uytterhoeven @ 2021-07-01 15:51 ` Sean Anderson 2021-07-01 16:44 ` Geert Uytterhoeven 0 siblings, 1 reply; 9+ messages in thread From: Sean Anderson @ 2021-07-01 15:51 UTC (permalink / raw) To: Geert Uytterhoeven, Luca Ceresoli Cc: linux-clk, Adam Ford, Stephen Boyd, Michael Turquette, Rob Herring, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin 2021-07-01 15:51 ` Sean Anderson @ 2021-07-01 16:44 ` Geert Uytterhoeven 2021-07-02 14:12 ` Luca Ceresoli 0 siblings, 1 reply; 9+ messages in thread From: Geert Uytterhoeven @ 2021-07-01 16:44 UTC (permalink / raw) To: Sean Anderson Cc: Luca Ceresoli, linux-clk, Adam Ford, Stephen Boyd, Michael Turquette, Rob Herring, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin 2021-07-01 16:44 ` Geert Uytterhoeven @ 2021-07-02 14:12 ` Luca Ceresoli 0 siblings, 0 replies; 9+ messages in thread From: Luca Ceresoli @ 2021-07-02 14:12 UTC (permalink / raw) To: Geert Uytterhoeven, Sean Anderson Cc: linux-clk, Adam Ford, Stephen Boyd, Michael Turquette, Rob Herring, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-07-02 14:12 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-29 15:47 [PATCH v3 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin Sean Anderson 2021-06-29 20:29 ` Stephen Boyd 2021-06-29 21:23 ` Luca Ceresoli 2021-06-29 21:41 ` Sean Anderson 2021-06-30 7:57 ` Luca Ceresoli 2021-06-30 9:12 ` Geert Uytterhoeven 2021-07-01 15:51 ` Sean Anderson 2021-07-01 16:44 ` Geert Uytterhoeven 2021-07-02 14:12 ` Luca Ceresoli
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).