linux-renesas-soc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH v2 2/2] clk: vc5: Add properties for configuring SD/OE behavior
       [not found]     ` <3174eed5-1078-68c4-4d98-95c448cd0940@seco.com>
@ 2021-06-29 12:42       ` Geert Uytterhoeven
  2021-06-29 15:49         ` Sean Anderson
  0 siblings, 1 reply; 2+ messages in thread
From: Geert Uytterhoeven @ 2021-06-29 12:42 UTC (permalink / raw)
  To: Sean Anderson
  Cc: Luca Ceresoli, linux-clk, Michael Turquette, Adam Ford,
	Stephen Boyd, Kieran Bingham, Laurent Pinchart, Linux-Renesas,
	Rob Herring,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

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

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH v2 2/2] clk: vc5: Add properties for configuring SD/OE behavior
  2021-06-29 12:42       ` [PATCH v2 2/2] clk: vc5: Add properties for configuring SD/OE behavior Geert Uytterhoeven
@ 2021-06-29 15:49         ` Sean Anderson
  0 siblings, 0 replies; 2+ messages in thread
From: Sean Anderson @ 2021-06-29 15:49 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Luca Ceresoli, linux-clk, Michael Turquette, Adam Ford,
	Stephen Boyd, Kieran Bingham, Laurent Pinchart, Linux-Renesas,
	Rob Herring,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS



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
> 

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2021-06-29 15:50 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20210614155437.3979771-1-sean.anderson@seco.com>
     [not found] ` <20210614155437.3979771-2-sean.anderson@seco.com>
     [not found]   ` <47b37414-6587-0792-201b-e255feeee9c9@lucaceresoli.net>
     [not found]     ` <3174eed5-1078-68c4-4d98-95c448cd0940@seco.com>
2021-06-29 12:42       ` [PATCH v2 2/2] clk: vc5: Add properties for configuring SD/OE behavior Geert Uytterhoeven
2021-06-29 15:49         ` Sean Anderson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).