Re: [PATCH v2 2/2] clk: vc5: Add properties for configuring SD/OE behavior

[Geert Uytterhoeven <geert@linux-m68k.org>]

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