* [PATCH v2 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin @ 2021-06-14 15:54 Sean Anderson 2021-06-14 15:54 ` [PATCH v2 2/2] clk: vc5: Add properties for configuring SD/OE behavior Sean Anderson ` (3 more replies) 0 siblings, 4 replies; 10+ messages in thread From: Sean Anderson @ 2021-06-14 15:54 UTC (permalink / raw) To: linux-clk, Luca Ceresoli Cc: Michael Turquette, Adam Ford, Stephen Boyd, 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> --- 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(+) 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 -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] clk: vc5: Add properties for configuring SD/OE behavior 2021-06-14 15:54 [PATCH v2 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin Sean Anderson @ 2021-06-14 15:54 ` Sean Anderson 2021-06-16 15:41 ` Luca Ceresoli 2021-06-28 2:31 ` Stephen Boyd 2021-06-16 15:41 ` [PATCH v2 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin Luca Ceresoli ` (2 subsequent siblings) 3 siblings, 2 replies; 10+ messages in thread From: Sean Anderson @ 2021-06-14 15:54 UTC (permalink / raw) To: linux-clk, Luca Ceresoli Cc: Michael Turquette, Adam Ford, Stephen Boyd, Sean Anderson 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> --- Changes in v2: - Set SH as well as SP drivers/clk/clk-versaclock5.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c index 344cd6c61188..09a96d34bac7 100644 --- a/drivers/clk/clk-versaclock5.c +++ b/drivers/clk/clk-versaclock5.c @@ -886,6 +886,7 @@ static const struct of_device_id clk_vc5_of_match[]; static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id) { + bool oe_polarity, sd_enable; struct vc5_driver_data *vc5; struct clk_init_data init; const char *parent_names[2]; @@ -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)); + /* Register clock input mux */ memset(&init, 0, sizeof(init)); -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] clk: vc5: Add properties for configuring SD/OE behavior 2021-06-14 15:54 ` [PATCH v2 2/2] clk: vc5: Add properties for configuring SD/OE behavior Sean Anderson @ 2021-06-16 15:41 ` Luca Ceresoli 2021-06-17 14:46 ` Sean Anderson 2021-06-28 2:31 ` Stephen Boyd 1 sibling, 1 reply; 10+ messages in thread From: Luca Ceresoli @ 2021-06-16 15:41 UTC (permalink / raw) To: Sean Anderson, linux-clk; +Cc: Michael Turquette, Adam Ford, Stephen Boyd Hi Sean, 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? Thanks, -- Luca ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] clk: vc5: Add properties for configuring SD/OE behavior 2021-06-16 15:41 ` Luca Ceresoli @ 2021-06-17 14:46 ` Sean Anderson 2021-06-29 12:42 ` Geert Uytterhoeven 0 siblings, 1 reply; 10+ messages in thread From: Sean Anderson @ 2021-06-17 14:46 UTC (permalink / raw) To: Luca Ceresoli, linux-clk; +Cc: Michael Turquette, Adam Ford, Stephen Boyd On 6/16/21 11:41 AM, Luca Ceresoli wrote: > Hi Sean, > > 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. --Sean ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] clk: vc5: Add properties for configuring SD/OE behavior 2021-06-17 14:46 ` Sean Anderson @ 2021-06-29 12:42 ` Geert Uytterhoeven 2021-06-29 15:49 ` Sean Anderson 0 siblings, 1 reply; 10+ messages in thread From: Geert Uytterhoeven @ 2021-06-29 12:42 UTC (permalink / raw) To: Sean Anderson Cc: Luca Ceresoli, linux-clk, Michael Turquette, Adam Ford, Stephen Boyd, Kieran Bingham, Laurent Pinchart, Linux-Renesas, Rob Herring, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] clk: vc5: Add properties for configuring SD/OE behavior 2021-06-29 12:42 ` Geert Uytterhoeven @ 2021-06-29 15:49 ` Sean Anderson 0 siblings, 0 replies; 10+ messages in thread From: Sean Anderson @ 2021-06-29 15:49 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Luca Ceresoli, linux-clk, Michael Turquette, Adam Ford, Stephen Boyd, Kieran Bingham, Laurent Pinchart, Linux-Renesas, Rob Herring, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] clk: vc5: Add properties for configuring SD/OE behavior 2021-06-14 15:54 ` [PATCH v2 2/2] clk: vc5: Add properties for configuring SD/OE behavior Sean Anderson 2021-06-16 15:41 ` Luca Ceresoli @ 2021-06-28 2:31 ` Stephen Boyd 1 sibling, 0 replies; 10+ messages in thread From: Stephen Boyd @ 2021-06-28 2:31 UTC (permalink / raw) To: Luca Ceresoli, Sean Anderson, linux-clk Cc: Michael Turquette, Adam Ford, Sean Anderson Quoting Sean Anderson (2021-06-14 08:54:37) > 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> > --- Applied to clk-next ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin 2021-06-14 15:54 [PATCH v2 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin Sean Anderson 2021-06-14 15:54 ` [PATCH v2 2/2] clk: vc5: Add properties for configuring SD/OE behavior Sean Anderson @ 2021-06-16 15:41 ` Luca Ceresoli 2021-06-24 20:37 ` Rob Herring 2021-06-28 2:31 ` Stephen Boyd 3 siblings, 0 replies; 10+ messages in thread From: Luca Ceresoli @ 2021-06-16 15:41 UTC (permalink / raw) To: Sean Anderson, linux-clk Cc: Michael Turquette, Adam Ford, Stephen Boyd, Rob Herring, devicetree 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> -- Luca ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin 2021-06-14 15:54 [PATCH v2 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin Sean Anderson 2021-06-14 15:54 ` [PATCH v2 2/2] clk: vc5: Add properties for configuring SD/OE behavior Sean Anderson 2021-06-16 15:41 ` [PATCH v2 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin Luca Ceresoli @ 2021-06-24 20:37 ` Rob Herring 2021-06-28 2:31 ` Stephen Boyd 3 siblings, 0 replies; 10+ messages in thread From: Rob Herring @ 2021-06-24 20:37 UTC (permalink / raw) To: Sean Anderson Cc: devicetree, Luca Ceresoli, linux-clk, Adam Ford, Stephen Boyd, Michael Turquette 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin 2021-06-14 15:54 [PATCH v2 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin Sean Anderson ` (2 preceding siblings ...) 2021-06-24 20:37 ` Rob Herring @ 2021-06-28 2:31 ` Stephen Boyd 3 siblings, 0 replies; 10+ messages in thread From: Stephen Boyd @ 2021-06-28 2:31 UTC (permalink / raw) To: Luca Ceresoli, Sean Anderson, linux-clk Cc: Michael Turquette, Adam Ford, Sean Anderson, Rob Herring, devicetree 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-06-29 15:50 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-14 15:54 [PATCH v2 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin Sean Anderson 2021-06-14 15:54 ` [PATCH v2 2/2] clk: vc5: Add properties for configuring SD/OE behavior Sean Anderson 2021-06-16 15:41 ` Luca Ceresoli 2021-06-17 14:46 ` Sean Anderson 2021-06-29 12:42 ` Geert Uytterhoeven 2021-06-29 15:49 ` Sean Anderson 2021-06-28 2:31 ` Stephen Boyd 2021-06-16 15:41 ` [PATCH v2 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin Luca Ceresoli 2021-06-24 20:37 ` Rob Herring 2021-06-28 2:31 ` Stephen Boyd
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.