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

- From: Sean Anderson \<hidden\>
- Date: 2021-06-29 15:50:09 
- List: linux-devicetree
- Page: https://ratatoskr.run/linux-devicetree/2021/06/8192560
- Thread: https://ratatoskr.run/linux-clk/2021/06/8192062/t.md

```text


On 6/29/21 8:42 AM, Geert Uytterhoeven wrote:
> Hi Sean,
>
> On Thu, Jun 17, 2021 at 4:53 PM Sean Anderson [ref] 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 [ref]
>>  >
>>  > Reviewed-by: Luca Ceresoli [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
>
```