On 6/29/21 8:42 AM, Geert Uytterhoeven wrote:

Hi Sean,

On Thu, Jun 17, 2021 at 4:53 PM Sean Anderson [off-list ref] wrote:

quoted

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 [off-list ref]
 >
 > Reviewed-by: Luca Ceresoli [off-list ref]
 >
 >> @@ -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