Hi Sean,
On Thu, Jun 17, 2021 at 4:53 PM Sean Anderson <sean.anderson@xxxxxxxx> 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@xxxxxxxx>
> >
> > Reviewed-by: Luca Ceresoli <luca@xxxxxxxxxxxxxxxx>
> >
> >> @@ -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@xxxxxxxxxxxxxx
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
