[3/4] dt-bindings: reset: uniphier: Add AHCI core reset description
diff mbox series

Message ID 1541727727-10821-4-git-send-email-hayashi.kunihiko@socionext.com
State New, archived
Headers show
Series
  • reset: uniphier: Rename from USB3 reset to glue reset and add AHCI reset support
Related show

Commit Message

Kunihiko Hayashi Nov. 9, 2018, 1:42 a.m. UTC
Add compatible strings for reset control of AHCI core implemented in
UniPhier SoCs. The reset control belongs to AHCI glue layer.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 Documentation/devicetree/bindings/reset/uniphier-reset.txt | 3 +++
 1 file changed, 3 insertions(+)

Comments

Philipp Zabel Nov. 9, 2018, 3:01 p.m. UTC | #1
Hi Kunihiko,

On Fri, 2018-11-09 at 10:42 +0900, Kunihiko Hayashi wrote:
> Add compatible strings for reset control of AHCI core implemented in
> UniPhier SoCs. The reset control belongs to AHCI glue layer.
> 
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
>  Documentation/devicetree/bindings/reset/uniphier-reset.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/reset/uniphier-reset.txt b/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> index f63c511..ea00517 100644
> --- a/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> +++ b/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> @@ -133,6 +133,9 @@ Required properties:
>      "socionext,uniphier-pxs2-usb3-reset" - for PXs2 SoC USB3
>      "socionext,uniphier-ld20-usb3-reset" - for LD20 SoC USB3
>      "socionext,uniphier-pxs3-usb3-reset" - for PXs3 SoC USB3
> +    "socionext,uniphier-pro4-ahci-reset" - for Pro4 SoC AHCI
> +    "socionext,uniphier-pxs2-ahci-reset" - for PXs2 SoC AHCI
> +    "socionext,uniphier-pxs3-ahci-reset" - for PXs3 SoC AHCI

Since the driver behaves identically for "socionext,uniphier-pro4-usb3-
reset" and "socionext,uniphier-pro4-ahci-reset", would it make sense to
add a common compatible?
Something like:
"socionext,uniphier-pro4-usb3-reset", "socionext,uniphier-pro4-glue-reset" - for USB3 SoC AHCI
"socionext,uniphier-pro4-ahci-reset", "socionext,uniphier-pro4-glue-reset" - for Pro4 SoC AHCI

That way if more places turn up where the glue layer reset is used,
you can add them without patching the driver every time.

regards
Philipp
Masahiro Yamada Nov. 9, 2018, 4:14 p.m. UTC | #2
On Sat, Nov 10, 2018 at 12:02 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> Hi Kunihiko,
>
> On Fri, 2018-11-09 at 10:42 +0900, Kunihiko Hayashi wrote:
> > Add compatible strings for reset control of AHCI core implemented in
> > UniPhier SoCs. The reset control belongs to AHCI glue layer.
> >
> > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> > ---
> >  Documentation/devicetree/bindings/reset/uniphier-reset.txt | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/reset/uniphier-reset.txt b/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> > index f63c511..ea00517 100644
> > --- a/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> > +++ b/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> > @@ -133,6 +133,9 @@ Required properties:
> >      "socionext,uniphier-pxs2-usb3-reset" - for PXs2 SoC USB3
> >      "socionext,uniphier-ld20-usb3-reset" - for LD20 SoC USB3
> >      "socionext,uniphier-pxs3-usb3-reset" - for PXs3 SoC USB3
> > +    "socionext,uniphier-pro4-ahci-reset" - for Pro4 SoC AHCI
> > +    "socionext,uniphier-pxs2-ahci-reset" - for PXs2 SoC AHCI
> > +    "socionext,uniphier-pxs3-ahci-reset" - for PXs3 SoC AHCI
>
> Since the driver behaves identically for "socionext,uniphier-pro4-usb3-
> reset" and "socionext,uniphier-pro4-ahci-reset", would it make sense to
> add a common compatible?


As far as I could guess, he just happened to find the same driver code
could be reused for other hardware.

Theoretically, this can happen anywhere since
a reset controller is just a set of registers
each bit of which is connected to a reset line.

If you added a super-generic compatible like "simple-reset",
I would agree with
"socionext,uniphier-pro4-usb3-reset", "simple-reset"
since this is a pattern.


However,
"socionext,uniphier-pro4-glue-reset" is kind of a halfway house
where it is SoC-specific, but still ambiguous.


> Something like:
> "socionext,uniphier-pro4-usb3-reset", "socionext,uniphier-pro4-glue-reset" - for USB3 SoC AHCI
> "socionext,uniphier-pro4-ahci-reset", "socionext,uniphier-pro4-glue-reset" - for Pro4 SoC AHCI
>
> That way if more places turn up where the glue layer reset is used,
> you can add them without patching the driver every time.


This is a trade-off between "patch the driver"
and "potential change of the binding".

There is no real hardware like pro4-glue-reset.



I am guessing this is a part of syscon or something,
but I cannot find any explanation in a bigger picture.

So, I cannot judge this further more.




> regards
> Philipp



--
Best Regards
Masahiro Yamada
Kunihiko Hayashi Nov. 12, 2018, 12:02 p.m. UTC | #3
Hi,

Thank you for some comments and pointing out.

On Sat, 10 Nov 2018 01:14:06 +0900 <yamada.masahiro@socionext.com> wrote:

> On Sat, Nov 10, 2018 at 12:02 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> >
> > Hi Kunihiko,
> >
> > On Fri, 2018-11-09 at 10:42 +0900, Kunihiko Hayashi wrote:
> > > Add compatible strings for reset control of AHCI core implemented in
> > > UniPhier SoCs. The reset control belongs to AHCI glue layer.
> > >
> > > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> > > ---
> > >  Documentation/devicetree/bindings/reset/uniphier-reset.txt | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/reset/uniphier-reset.txt b/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> > > index f63c511..ea00517 100644
> > > --- a/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> > > +++ b/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> > > @@ -133,6 +133,9 @@ Required properties:
> > >      "socionext,uniphier-pxs2-usb3-reset" - for PXs2 SoC USB3
> > >      "socionext,uniphier-ld20-usb3-reset" - for LD20 SoC USB3
> > >      "socionext,uniphier-pxs3-usb3-reset" - for PXs3 SoC USB3
> > > +    "socionext,uniphier-pro4-ahci-reset" - for Pro4 SoC AHCI
> > > +    "socionext,uniphier-pxs2-ahci-reset" - for PXs2 SoC AHCI
> > > +    "socionext,uniphier-pxs3-ahci-reset" - for PXs3 SoC AHCI
> >
> > Since the driver behaves identically for "socionext,uniphier-pro4-usb3-
> > reset" and "socionext,uniphier-pro4-ahci-reset", would it make sense to
> > add a common compatible?
> 
> 
> As far as I could guess, he just happened to find the same driver code
> could be reused for other hardware.
> 
> Theoretically, this can happen anywhere since
> a reset controller is just a set of registers
> each bit of which is connected to a reset line.
> 
> If you added a super-generic compatible like "simple-reset",
> I would agree with
> "socionext,uniphier-pro4-usb3-reset", "simple-reset"
> since this is a pattern.

I think it's more generic to define simple-reset with parent clock/reset
control without both SoC and device names.

However, such parent clocks/resets strongly depends on SoC, 
so I think it's difficut to lead generic definition in this case.

If we add generic compatible string, I also add "simple-reset".


> However,
> "socionext,uniphier-pro4-glue-reset" is kind of a halfway house
> where it is SoC-specific, but still ambiguous.

Surely, it might be hard to understand that pro4-glue-reset is SoC-specific
but for generic-device.


> > Something like:
> > "socionext,uniphier-pro4-usb3-reset", "socionext,uniphier-pro4-glue-reset" - for USB3 SoC AHCI
> > "socionext,uniphier-pro4-ahci-reset", "socionext,uniphier-pro4-glue-reset" - for Pro4 SoC AHCI
> >
> > That way if more places turn up where the glue layer reset is used,
> > you can add them without patching the driver every time.
> 
> 
> This is a trade-off between "patch the driver"
> and "potential change of the binding".

Adding "glue-reset" is usable for devices having same parent clocks/resets as
usb3/ahci, and we can add the devices without patches.

However, if the device needs other parent clocks/resets for the same SoC,
we can't add "glue-reset" and we might add patches for the device as a result.
In this case, the "glue-reset" will become difficult to understand.


> There is no real hardware like pro4-glue-reset.
>
> I am guessing this is a part of syscon or something,
> but I cannot find any explanation in a bigger picture.
> 
> So, I cannot judge this further more.

Since it's hard to lead the best result,
currently I'd like to suggest the current compatibles, with "simple-reset"
if necessary.

Thank you,

---
Best Regards,
Kunihiko Hayashi
Philipp Zabel Nov. 12, 2018, 2:21 p.m. UTC | #4
Hi,

On Mon, 2018-11-12 at 21:02 +0900, Kunihiko Hayashi wrote:
> Hi,
> 
> Thank you for some comments and pointing out.
> 
> On Sat, 10 Nov 2018 01:14:06 +0900 <yamada.masahiro@socionext.com> wrote:
> 
> > On Sat, Nov 10, 2018 at 12:02 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > 
> > > Hi Kunihiko,
> > > 
> > > On Fri, 2018-11-09 at 10:42 +0900, Kunihiko Hayashi wrote:
> > > > Add compatible strings for reset control of AHCI core implemented in
> > > > UniPhier SoCs. The reset control belongs to AHCI glue layer.
> > > > 
> > > > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> > > > ---
> > > >  Documentation/devicetree/bindings/reset/uniphier-reset.txt | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/reset/uniphier-reset.txt b/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> > > > index f63c511..ea00517 100644
> > > > --- a/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> > > > +++ b/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> > > > @@ -133,6 +133,9 @@ Required properties:
> > > >      "socionext,uniphier-pxs2-usb3-reset" - for PXs2 SoC USB3
> > > >      "socionext,uniphier-ld20-usb3-reset" - for LD20 SoC USB3
> > > >      "socionext,uniphier-pxs3-usb3-reset" - for PXs3 SoC USB3
> > > > +    "socionext,uniphier-pro4-ahci-reset" - for Pro4 SoC AHCI
> > > > +    "socionext,uniphier-pxs2-ahci-reset" - for PXs2 SoC AHCI
> > > > +    "socionext,uniphier-pxs3-ahci-reset" - for PXs3 SoC AHCI
> > > 
> > > Since the driver behaves identically for "socionext,uniphier-pro4-usb3-
> > > reset" and "socionext,uniphier-pro4-ahci-reset", would it make sense to
> > > add a common compatible?
> > 
> > As far as I could guess, he just happened to find the same driver code
> > could be reused for other hardware.

Ok, in that case never mind. I just assumed that this could be a case of
glue layer building blocks being reused by the hardware engineers, since
the driver reuses the same clock names for USB3 and AHCI both on Pro4
and on PXs2.

> > Theoretically, this can happen anywhere since
> > a reset controller is just a set of registers
> > each bit of which is connected to a reset line.
> > 
> > If you added a super-generic compatible like "simple-reset",
> > I would agree with
> > "socionext,uniphier-pro4-usb3-reset", "simple-reset"
> > since this is a pattern.
> 
> I think it's more generic to define simple-reset with parent clock/reset
> control without both SoC and device names.
> 
> However, such parent clocks/resets strongly depends on SoC, 
> so I think it's difficut to lead generic definition in this case.
> 
> If we add generic compatible string, I also add "simple-reset".

There is no "simple-reset" binding definition. As soon as there are SoC
specific clocks, it's not really generic anymore.

> > However,
> > "socionext,uniphier-pro4-glue-reset" is kind of a halfway house
> > where it is SoC-specific, but still ambiguous.
> 
> Surely, it might be hard to understand that pro4-glue-reset is SoC-specific
> but for generic-device.

I agree.

> > > Something like:
> > > "socionext,uniphier-pro4-usb3-reset", "socionext,uniphier-pro4-glue-reset" - for USB3 SoC AHCI
> > > "socionext,uniphier-pro4-ahci-reset", "socionext,uniphier-pro4-glue-reset" - for Pro4 SoC AHCI
> > > 
> > > That way if more places turn up where the glue layer reset is used,
> > > you can add them without patching the driver every time.
> > 
> > 
> > This is a trade-off between "patch the driver"
> > and "potential change of the binding".
> 
> Adding "glue-reset" is usable for devices having same parent clocks/resets as
> usb3/ahci, and we can add the devices without patches.
> 
> However, if the device needs other parent clocks/resets for the same SoC,
> we can't add "glue-reset" and we might add patches for the device as a result.
> In this case, the "glue-reset" will become difficult to understand.

Then let's not bother with it.
I made the suggestion without knowing the full picture.

> > There is no real hardware like pro4-glue-reset.
> > 
> > I am guessing this is a part of syscon or something,
> > but I cannot find any explanation in a bigger picture.
> > 
> > So, I cannot judge this further more.
> 
> Since it's hard to lead the best result,
> currently I'd like to suggest the current compatibles,

That is fine with me.

> with "simple-reset" if necessary.

Not necessary.

best regards
Philipp
Kunihiko Hayashi Nov. 13, 2018, 12:57 a.m. UTC | #5
Hi Philipp,

On Mon, 12 Nov 2018 15:21:46 +0100 <p.zabel@pengutronix.de> wrote:

> Hi,
> 
> On Mon, 2018-11-12 at 21:02 +0900, Kunihiko Hayashi wrote:
> > Hi,
> > 
> > Thank you for some comments and pointing out.
> > 
> > On Sat, 10 Nov 2018 01:14:06 +0900 <yamada.masahiro@socionext.com> wrote:
> > 
> > > On Sat, Nov 10, 2018 at 12:02 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > > 
> > > > Hi Kunihiko,
> > > > 
> > > > On Fri, 2018-11-09 at 10:42 +0900, Kunihiko Hayashi wrote:
> > > > > Add compatible strings for reset control of AHCI core implemented in
> > > > > UniPhier SoCs. The reset control belongs to AHCI glue layer.
> > > > > 
> > > > > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/reset/uniphier-reset.txt | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/reset/uniphier-reset.txt b/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> > > > > index f63c511..ea00517 100644
> > > > > --- a/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> > > > > +++ b/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> > > > > @@ -133,6 +133,9 @@ Required properties:
> > > > >      "socionext,uniphier-pxs2-usb3-reset" - for PXs2 SoC USB3
> > > > >      "socionext,uniphier-ld20-usb3-reset" - for LD20 SoC USB3
> > > > >      "socionext,uniphier-pxs3-usb3-reset" - for PXs3 SoC USB3
> > > > > +    "socionext,uniphier-pro4-ahci-reset" - for Pro4 SoC AHCI
> > > > > +    "socionext,uniphier-pxs2-ahci-reset" - for PXs2 SoC AHCI
> > > > > +    "socionext,uniphier-pxs3-ahci-reset" - for PXs3 SoC AHCI
> > > > 
> > > > Since the driver behaves identically for "socionext,uniphier-pro4-usb3-
> > > > reset" and "socionext,uniphier-pro4-ahci-reset", would it make sense to
> > > > add a common compatible?
> > > 
> > > As far as I could guess, he just happened to find the same driver code
> > > could be reused for other hardware.
> 
> Ok, in that case never mind. I just assumed that this could be a case of
> glue layer building blocks being reused by the hardware engineers, since
> the driver reuses the same clock names for USB3 and AHCI both on Pro4
> and on PXs2.
> 
> > > Theoretically, this can happen anywhere since
> > > a reset controller is just a set of registers
> > > each bit of which is connected to a reset line.
> > > 
> > > If you added a super-generic compatible like "simple-reset",
> > > I would agree with
> > > "socionext,uniphier-pro4-usb3-reset", "simple-reset"
> > > since this is a pattern.
> > 
> > I think it's more generic to define simple-reset with parent clock/reset
> > control without both SoC and device names.
> > 
> > However, such parent clocks/resets strongly depends on SoC, 
> > so I think it's difficut to lead generic definition in this case.
> > 
> > If we add generic compatible string, I also add "simple-reset".
> 
> There is no "simple-reset" binding definition. As soon as there are SoC
> specific clocks, it's not really generic anymore.

I see. I understand "simple-reset" doesn't need.

> > > However,
> > > "socionext,uniphier-pro4-glue-reset" is kind of a halfway house
> > > where it is SoC-specific, but still ambiguous.
> > 
> > Surely, it might be hard to understand that pro4-glue-reset is SoC-specific
> > but for generic-device.
> 
> I agree.
> 
> > > > Something like:
> > > > "socionext,uniphier-pro4-usb3-reset", "socionext,uniphier-pro4-glue-reset" - for USB3 SoC AHCI
> > > > "socionext,uniphier-pro4-ahci-reset", "socionext,uniphier-pro4-glue-reset" - for Pro4 SoC AHCI
> > > > 
> > > > That way if more places turn up where the glue layer reset is used,
> > > > you can add them without patching the driver every time.
> > > 
> > > 
> > > This is a trade-off between "patch the driver"
> > > and "potential change of the binding".
> > 
> > Adding "glue-reset" is usable for devices having same parent clocks/resets as
> > usb3/ahci, and we can add the devices without patches.
> > 
> > However, if the device needs other parent clocks/resets for the same SoC,
> > we can't add "glue-reset" and we might add patches for the device as a result.
> > In this case, the "glue-reset" will become difficult to understand.
> 
> Then let's not bother with it.
> I made the suggestion without knowing the full picture.

This might confuse you without bigger picture.

> > > There is no real hardware like pro4-glue-reset.
> > > 
> > > I am guessing this is a part of syscon or something,
> > > but I cannot find any explanation in a bigger picture.
> > > 
> > > So, I cannot judge this further more.
> > 
> > Since it's hard to lead the best result,
> > currently I'd like to suggest the current compatibles,
> 
> That is fine with me.
> 
> > with "simple-reset" if necessary.
> 
> Not necessary.

Okay, I don't add it.

Thank you,

---
Best Regards,
Kunihiko Hayashi
Rob Herring Nov. 17, 2018, 4:30 p.m. UTC | #6
On Fri,  9 Nov 2018 10:42:06 +0900, Kunihiko Hayashi wrote:
> Add compatible strings for reset control of AHCI core implemented in
> UniPhier SoCs. The reset control belongs to AHCI glue layer.
> 
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
>  Documentation/devicetree/bindings/reset/uniphier-reset.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/reset/uniphier-reset.txt b/Documentation/devicetree/bindings/reset/uniphier-reset.txt
index f63c511..ea00517 100644
--- a/Documentation/devicetree/bindings/reset/uniphier-reset.txt
+++ b/Documentation/devicetree/bindings/reset/uniphier-reset.txt
@@ -133,6 +133,9 @@  Required properties:
     "socionext,uniphier-pxs2-usb3-reset" - for PXs2 SoC USB3
     "socionext,uniphier-ld20-usb3-reset" - for LD20 SoC USB3
     "socionext,uniphier-pxs3-usb3-reset" - for PXs3 SoC USB3
+    "socionext,uniphier-pro4-ahci-reset" - for Pro4 SoC AHCI
+    "socionext,uniphier-pxs2-ahci-reset" - for PXs2 SoC AHCI
+    "socionext,uniphier-pxs3-ahci-reset" - for PXs3 SoC AHCI
 - #reset-cells: Should be 1.
 - reg: Specifies offset and length of the register set for the device.
 - clocks: A list of phandles to the clock gate for the glue layer.