| Message ID | 20210614155437.3979771-2-sean.anderson@seco.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [v2,1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin | expand |
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,
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
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
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/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));
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(+)