From: Sean Anderson <hidden> Date: 2021-06-29 15:48:48
These properties allow configuring the SD/OE pin as described in the
datasheet.
Signed-off-by: Sean Anderson <redacted>
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(+)
@@ -30,6 +30,22 @@ description: |3 -- OUT34 -- 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:22760description: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
From: Stephen Boyd <sboyd@kernel.org> Date: 2021-06-29 20:29:57
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 <redacted>
Acked-by: Rob Herring <robh@kernel.org>
---
I dropped v2 from clk-next and this will be punted to the next release.
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 <redacted>
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.
quoted hunk
---
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(+)
@@ -30,6 +30,22 @@ description: |3 -- OUT34 -- 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.
quoted hunk
@@ -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
From: Sean Anderson <hidden> Date: 2021-06-29 21:41:48
On 6/29/21 5:23 PM, Luca Ceresoli wrote:
Hi Sean,
On 29/06/21 17:47, Sean Anderson wrote:
quoted
These properties allow configuring the SD/OE pin as described in the
datasheet.
*Many* thanks for addressing this issue so quickly!
quoted
Signed-off-by: Sean Anderson <redacted>
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.
quoted
---
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(+)
@@ -30,6 +30,22 @@ description: |3 -- OUT34 -- 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.
quoted
@@ -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-*).
These properties allow configuring the SD/OE pin as described in the
datasheet.
*Many* thanks for addressing this issue so quickly!
quoted
Signed-off-by: Sean Anderson <redacted>
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.
quoted
quoted
---
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
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.
quoted
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.
quoted
quoted
@@ -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
Hi Luca,
On Wed, Jun 30, 2021 at 9:57 AM Luca Ceresoli [off-list ref] wrote:
On 29/06/21 23:41, Sean Anderson wrote:
quoted
On 6/29/21 5:23 PM, Luca Ceresoli wrote:
quoted
On 29/06/21 17:47, Sean Anderson wrote:
quoted
These properties allow configuring the SD/OE pin as described in the
datasheet.
*Many* thanks for addressing this issue so quickly!
quoted
Signed-off-by: Sean Anderson <redacted>
quoted
quoted
quoted
a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
index 28675b0b80f1..51f0f78cc3f4 100644
@@ -30,6 +30,22 @@ description: |3 -- OUT34 -- 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
From: Sean Anderson <hidden> Date: 2021-07-01 15:52:06
On 6/30/21 5:12 AM, Geert Uytterhoeven wrote:
Hi Luca,
On Wed, Jun 30, 2021 at 9:57 AM Luca Ceresoli [off-list ref] wrote:
quoted
On 29/06/21 23:41, Sean Anderson wrote:
quoted
On 6/29/21 5:23 PM, Luca Ceresoli wrote:
quoted
On 29/06/21 17:47, Sean Anderson wrote:
quoted
These properties allow configuring the SD/OE pin as described in the
datasheet.
*Many* thanks for addressing this issue so quickly!
quoted
Signed-off-by: Sean Anderson <redacted>
quoted
quoted
quoted
quoted
a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
index 28675b0b80f1..51f0f78cc3f4 100644
@@ -30,6 +30,22 @@ description: |3 -- OUT34 -- 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.
quoted
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 [off-list ref] wrote:
On 6/30/21 5:12 AM, Geert Uytterhoeven wrote:
quoted
On Wed, Jun 30, 2021 at 9:57 AM Luca Ceresoli [off-list ref] wrote:
quoted
On 29/06/21 23:41, Sean Anderson wrote:
quoted
On 6/29/21 5:23 PM, Luca Ceresoli wrote:
quoted
On 29/06/21 17:47, Sean Anderson wrote:
quoted
These properties allow configuring the SD/OE pin as described in the
datasheet.
*Many* thanks for addressing this issue so quickly!
quoted
Signed-off-by: Sean Anderson <redacted>
quoted
quoted
quoted
quoted
a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
index 28675b0b80f1..51f0f78cc3f4 100644
@@ -30,6 +30,22 @@ description: |3 -- OUT34 -- 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.
quoted
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.
quoted
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:
[...]
quoted
quoted
quoted
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