| Message ID | 1541727727-10821-4-git-send-email-hayashi.kunihiko@socionext.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | reset: uniphier: Rename from USB3 reset to glue reset and add AHCI reset support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
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
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
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
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
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
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>
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.
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(+)