[v4,5/5] dt-bindings: clock: ast2600: Add reset config for I3C

Add reset line definitions for the AST2600 I3C block's reset inputs.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---
v2:
 - reword commit message
---
 include/dt-bindings/clock/ast2600-clock.h | 6 ++++++
 1 file changed, 6 insertions(+)

On Tue, 28 Feb 2023 at 09:16, Jeremy Kerr <jk@codeconstruct.com.au> wrote:
>
> Add reset line definitions for the AST2600 I3C block's reset inputs.
>
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
> ---
> v2:
>  - reword commit message
> ---
>  include/dt-bindings/clock/ast2600-clock.h | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/include/dt-bindings/clock/ast2600-clock.h b/include/dt-bindings/clock/ast2600-clock.h
> index b4d69103d722..b1c129977910 100644
> --- a/include/dt-bindings/clock/ast2600-clock.h
> +++ b/include/dt-bindings/clock/ast2600-clock.h
> @@ -90,6 +90,12 @@
>  /* Only list resets here that are not part of a gate */

These definitions are part of a gate, yeah?

>  #define ASPEED_RESET_ADC               55
>  #define ASPEED_RESET_JTAG_MASTER2      54
> +#define ASPEED_RESET_I3C5              45
> +#define ASPEED_RESET_I3C4              44
> +#define ASPEED_RESET_I3C3              43
> +#define ASPEED_RESET_I3C2              42
> +#define ASPEED_RESET_I3C1              41
> +#define ASPEED_RESET_I3C0              40
>  #define ASPEED_RESET_I3C_DMA           39
>  #define ASPEED_RESET_PWM               37
>  #define ASPEED_RESET_PECI              36
> --
> 2.39.1
>

Hi Joel,

> > diff --git a/include/dt-bindings/clock/ast2600-clock.h
> > b/include/dt-bindings/clock/ast2600-clock.h
> > index b4d69103d722..b1c129977910 100644
> > --- a/include/dt-bindings/clock/ast2600-clock.h
> > +++ b/include/dt-bindings/clock/ast2600-clock.h
> > @@ -90,6 +90,12 @@
> >  /* Only list resets here that are not part of a gate */
> 
> These definitions are part of a gate, yeah?

Well, no more "part of a gate" than all of the other definitions :)

All the defines in this section are references to individual bits in
the reset register banks in SCU040 & SCU050; the i3c set are the same
as the others there.

So I'm not sure what that comment is supposed to signify as to what
qualifies as a "gate" in the context of a reset...

Cheers,


Jeremy

On Wed, 1 Mar 2023 at 06:29, Jeremy Kerr <jk@codeconstruct.com.au> wrote:
>
> Hi Joel,
>
> > > diff --git a/include/dt-bindings/clock/ast2600-clock.h
> > > b/include/dt-bindings/clock/ast2600-clock.h
> > > index b4d69103d722..b1c129977910 100644
> > > --- a/include/dt-bindings/clock/ast2600-clock.h
> > > +++ b/include/dt-bindings/clock/ast2600-clock.h
> > > @@ -90,6 +90,12 @@
> > >  /* Only list resets here that are not part of a gate */
> >
> > These definitions are part of a gate, yeah?
>
> Well, no more "part of a gate" than all of the other definitions :)
>
> All the defines in this section are references to individual bits in
> the reset register banks in SCU040 & SCU050; the i3c set are the same
> as the others there.
>
> So I'm not sure what that comment is supposed to signify as to what
> qualifies as a "gate" in the context of a reset...

This is poor documentation from the author of the clock driver, which is me.

We only expose the reset lines in the device tree for resets that are
not associated with a clock line.

This is done because the aspeed docs specify we do a dance when enabling an IP:

 1. Place IP in reset
 2. Enable clock
 3. Delay
 4. Release reset

So we do this with the aspeed_g6_gates array. The rule is: any gate
with a number in the rst column doesn't have that reset line exposed.
That's what this cryptic comment in the header is warning about.

This was documented to some extent in the original commit message for
the 2400/2500 driver:

 https://git.kernel.org/torvalds/c/15ed8ce5f84e2b

We could hoist that out and put it in the source file(s).

Cheers,

Joel

Hi Joel,

> > So I'm not sure what that comment is supposed to signify as to what
> > qualifies as a "gate" in the context of a reset...
> 
> This is poor documentation from the author of the clock driver,

Hah, not that guy again!

> which is me.

oh.

> We only expose the reset lines in the device tree for resets that are
> not associated with a clock line.
> 
> This is done because the aspeed docs specify we do a dance when
> enabling an IP:
> 
>  1. Place IP in reset
>  2. Enable clock
>  3. Delay
>  4. Release reset
> 
> So we do this with the aspeed_g6_gates array. The rule is: any gate
> with a number in the rst column doesn't have that reset line exposed.
> That's what this cryptic comment in the header is warning about.

That makes sense, and means I can drop the explicit reset control from
the DTS, and then we don't need these definitions.

> This was documented to some extent in the original commit message for
> the 2400/2500 driver:
> 
>  https://git.kernel.org/torvalds/c/15ed8ce5f84e2b
> 
> We could hoist that out and put it in the source file(s).

Awesome, thanks for the explanation - I'll add a patch to do so.

Cheers,


Jeremy