diff mbox series

[v2,1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin

Message ID 20210614155437.3979771-1-sean.anderson@seco.com
State Not Applicable
Headers show
Series [v2,1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin | expand

Checks

Context Check Description
robh/checkpatch success
robh/dt-meta-schema success
robh/dtbs-check success

Commit Message

Sean Anderson June 14, 2021, 3:54 p.m. UTC
These properties allow configuring the SD/OE pin as described in the
datasheet.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

Changes in v2:
- Rename idt,sd-active-high to idt,output-enable-active-high
- Add idt,enable-shutdown

 .../bindings/clock/idt,versaclock5.yaml       | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)

Comments

Luca Ceresoli June 16, 2021, 3:41 p.m. UTC | #1
On 14/06/21 17:54, Sean Anderson wrote:
> These properties allow configuring the SD/OE pin as described in the
> datasheet.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>

Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
Rob Herring June 24, 2021, 8:37 p.m. UTC | #2
On Mon, 14 Jun 2021 11:54:36 -0400, Sean Anderson wrote:
> These properties allow configuring the SD/OE pin as described in the
> datasheet.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
> Changes in v2:
> - Rename idt,sd-active-high to idt,output-enable-active-high
> - Add idt,enable-shutdown
> 
>  .../bindings/clock/idt,versaclock5.yaml       | 33 +++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>
Stephen Boyd June 28, 2021, 2:31 a.m. UTC | #3
Quoting Sean Anderson (2021-06-14 08:54:36)
> These properties allow configuring the SD/OE pin as described in the
> datasheet.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---

Applied to clk-next
Geert Uytterhoeven June 29, 2021, 12:42 p.m. UTC | #4
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
Sean Anderson June 29, 2021, 3:49 p.m. UTC | #5
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
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
index 28675b0b80f1..79d67fad5284 100644
--- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
+++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
@@ -30,6 +30,22 @@  description: |
     3 -- OUT3
     4 -- OUT4
 
+  The idt,enable-shutdown and idt,output-enable-active-high properties
+  correspond to the SH and SP bits of the Primary Source and Shutdown
+  Register, respectively. Their behavior is summarized by the following
+  table:
+
+  SH SP SD/OE Output
+  == == ===== ========
+   0  0 Low   Active
+   0  0 High  Inactive
+   0  1 Low   Inactive
+   0  1 High  Active
+   1  0 Low   Active
+   1  0 High  Shutdown
+   1  1 Low   Inactive
+   1  1 High  Shutdown
+
 maintainers:
   - Luca Ceresoli <luca@lucaceresoli.net>
 
@@ -64,6 +80,23 @@  properties:
     maximum: 22760
     description: Optional load capacitor for XTAL1 and XTAL2
 
+  idt,enable-shutdown:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description: |
+      Enable the shutdown function when the SD/OE pin is high. This
+      corresponds to setting the SH bit of the Primary Source and
+      Shutdown Register. If this property is set, it takes precedence
+      over the usual enable/disable semantics of the SD/OE pin.
+
+  idt,output-enable-active-high:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description: |
+      This enables output when the SD/OE pin is high, and disables
+      output when the SD/OE pin is low. This corresponds to setting the
+      SP bit of the Primary Source and Shutdown Register. If this
+      property is not present, then the SD/OE pin has the opposite
+      polarity (enabled when low, disabled when high).
+
 patternProperties:
   "^OUT[1-4]$":
     type: object