| Message ID | 20210614155437.3979771-1-sean.anderson@seco.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v2,1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | success | |
| robh/dtbs-check | success |
On 14/06/21 17:54, Sean Anderson wrote: > These properties allow configuring the SD/OE pin as described in the > datasheet. > > Signed-off-by: Sean Anderson <sean.anderson@seco.com> Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
On Mon, 14 Jun 2021 11:54:36 -0400, Sean Anderson wrote: > These properties allow configuring the SD/OE pin as described in the > datasheet. > > Signed-off-by: Sean Anderson <sean.anderson@seco.com> > --- > > Changes in v2: > - Rename idt,sd-active-high to idt,output-enable-active-high > - Add idt,enable-shutdown > > .../bindings/clock/idt,versaclock5.yaml | 33 +++++++++++++++++++ > 1 file changed, 33 insertions(+) > Acked-by: Rob Herring <robh@kernel.org>
Quoting Sean Anderson (2021-06-14 08:54:36) > These properties allow configuring the SD/OE pin as described in the > datasheet. > > Signed-off-by: Sean Anderson <sean.anderson@seco.com> > --- Applied to clk-next
Hi Sean, On Thu, Jun 17, 2021 at 4:53 PM Sean Anderson <sean.anderson@seco.com> wrote: > On 6/16/21 11:41 AM, Luca Ceresoli wrote: > > On 14/06/21 17:54, Sean Anderson wrote: > >> The SD/OE pin may be configured to enable output when high or low, and > >> to shutdown the device when high. This behavior is controller by the SH > >> and SP bits of the Primary Source and Shutdown Register (and to a lesser > >> extent the OS and OE bits). By default, both bits are 0, but they may > >> need to be configured differently, depending on the external circuitry > >> controlling the SD/OE pin. > >> > >> Signed-off-by: Sean Anderson <sean.anderson@seco.com> > > > > Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net> > > > >> @@ -914,6 +915,15 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id) > >> return PTR_ERR(vc5->regmap); > >> } > >> > >> + oe_polarity = of_property_read_bool(client->dev.of_node, > >> + "idt,output-enable-active-high"); > >> + sd_enable = of_property_read_bool(client->dev.of_node, > >> + "idt,enable-shutdown"); > >> + regmap_update_bits(vc5->regmap, VC5_PRIM_SRC_SHDN, > >> + VC5_PRIM_SRC_SHDN_SP | VC5_PRIM_SRC_SHDN_EN_GBL_SHDN, > >> + (oe_polarity ? VC5_PRIM_SRC_SHDN_SP : 0) > >> + | (sd_enable ? VC5_PRIM_SRC_SHDN_EN_GBL_SHDN : 0)); > >> + > > > > Did you test all combinations? > > No. I only tested "idt,output-enable-active-high". Though I also in > effect tested the default case (both zero) as well. > > One potential impact of this patch could be that systems which enabled > the SP and SH bits via OTP could end up inadvertently disabling them > because they need to add the appropriate property. Maintaining full > backwards compatibility would require a tri-state property of some kind. And that seems to be exactly what's happening for me... I've just discovered a failure on Renesas Salvator-XS caused by this patch, which is now commit e26b493f3495e8a2 ("clk: vc5: Add properties for configuring SD/OE behavior") in clk-next: [dm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:76:crtc-3] flip_done timed out [drm:drm_crtc_commit_wait] *ERROR* flip_done timed out [...] Printing the value of VC5_PRIM_SRC_SHDN before/after the update: vc5 4-006a: vc5_probe:945: 0x8a vc5 4-006a: vc5_probe:951: 0x88 My initial bisection failed, as the register contents are retained across a reset. Hence booting a "good" kernel after a "bad" kernel still fails, unless the VC5 is power-cycled in between. So I think we do need separate "idt,output-enable-active-low" and "idt,disable-shutdown" properties, and not touch the bits if none of the corresponding properties is present. 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 6/29/21 8:42 AM, Geert Uytterhoeven wrote: > Hi Sean, > > On Thu, Jun 17, 2021 at 4:53 PM Sean Anderson <sean.anderson@seco.com> wrote: >> On 6/16/21 11:41 AM, Luca Ceresoli wrote: >> > On 14/06/21 17:54, Sean Anderson wrote: >> >> The SD/OE pin may be configured to enable output when high or low, and >> >> to shutdown the device when high. This behavior is controller by the SH >> >> and SP bits of the Primary Source and Shutdown Register (and to a lesser >> >> extent the OS and OE bits). By default, both bits are 0, but they may >> >> need to be configured differently, depending on the external circuitry >> >> controlling the SD/OE pin. >> >> >> >> Signed-off-by: Sean Anderson <sean.anderson@seco.com> >> > >> > Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net> >> > >> >> @@ -914,6 +915,15 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id) >> >> return PTR_ERR(vc5->regmap); >> >> } >> >> >> >> + oe_polarity = of_property_read_bool(client->dev.of_node, >> >> + "idt,output-enable-active-high"); >> >> + sd_enable = of_property_read_bool(client->dev.of_node, >> >> + "idt,enable-shutdown"); >> >> + regmap_update_bits(vc5->regmap, VC5_PRIM_SRC_SHDN, >> >> + VC5_PRIM_SRC_SHDN_SP | VC5_PRIM_SRC_SHDN_EN_GBL_SHDN, >> >> + (oe_polarity ? VC5_PRIM_SRC_SHDN_SP : 0) >> >> + | (sd_enable ? VC5_PRIM_SRC_SHDN_EN_GBL_SHDN : 0)); >> >> + >> > >> > Did you test all combinations? >> >> No. I only tested "idt,output-enable-active-high". Though I also in >> effect tested the default case (both zero) as well. >> >> One potential impact of this patch could be that systems which enabled >> the SP and SH bits via OTP could end up inadvertently disabling them >> because they need to add the appropriate property. Maintaining full >> backwards compatibility would require a tri-state property of some kind. > > And that seems to be exactly what's happening for me... > > I've just discovered a failure on Renesas Salvator-XS caused by this > patch, which is now commit e26b493f3495e8a2 ("clk: vc5: Add properties > for configuring SD/OE behavior") in clk-next: > > [dm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:76:crtc-3] > flip_done timed out > [drm:drm_crtc_commit_wait] *ERROR* flip_done timed out > [...] > > Printing the value of VC5_PRIM_SRC_SHDN before/after the update: > > vc5 4-006a: vc5_probe:945: 0x8a > vc5 4-006a: vc5_probe:951: 0x88 > > My initial bisection failed, as the register contents are retained > across a reset. Hence booting a "good" kernel after a "bad" kernel > still fails, unless the VC5 is power-cycled in between. > > So I think we do need separate "idt,output-enable-active-low" and > "idt,disable-shutdown" properties, and not touch the bits if none of > the corresponding properties is present. Ok, I've submitted a v3 with these properties added. --Sean > > 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 >
diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml index 28675b0b80f1..79d67fad5284 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,enable-shutdown and idt,output-enable-active-high properties + correspond to the SH and SP bits of the Primary Source and Shutdown + Register, respectively. Their behavior is summarized by the following + table: + + SH SP SD/OE Output + == == ===== ======== + 0 0 Low Active + 0 0 High Inactive + 0 1 Low Inactive + 0 1 High Active + 1 0 Low Active + 1 0 High Shutdown + 1 1 Low Inactive + 1 1 High Shutdown + maintainers: - Luca Ceresoli <luca@lucaceresoli.net> @@ -64,6 +80,23 @@ 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. If this property is set, it takes precedence + over the usual enable/disable semantics of the SD/OE pin. + + 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. If this + property is not present, then the SD/OE pin has the opposite + polarity (enabled when low, disabled when high). + patternProperties: "^OUT[1-4]$": type: object
These properties allow configuring the SD/OE pin as described in the datasheet. Signed-off-by: Sean Anderson <sean.anderson@seco.com> --- Changes in v2: - Rename idt,sd-active-high to idt,output-enable-active-high - Add idt,enable-shutdown .../bindings/clock/idt,versaclock5.yaml | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+)