| Message ID | 20220912085730.390555-1-piyush.mehta@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | dt-bindings: usb: dwc3: Add interrupt-names to include hibernation interrupt | expand |
Hello Krzysztof, Thanks for the review comments. Please find my inline comments with tag [Piyush]. Regards, Piyush Mehta > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Tuesday, September 13, 2022 2:52 PM > To: Mehta, Piyush <piyush.mehta@amd.com>; gregkh@linuxfoundation.org; > robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; balbi@kernel.org > Cc: linux-usb@vger.kernel.org; devicetree@vger.kernel.org; linux- > kernel@vger.kernel.org; Simek, Michal <michal.simek@amd.com>; Paladugu, > Siva Durga Prasad <siva.durga.prasad.paladugu@amd.com>; Manish Narani > <manish.narani@xilinx.com> > Subject: Re: [PATCH] dt-bindings: usb: dwc3: Add interrupt-names to include > hibernation interrupt > > On 12/09/2022 10:57, Piyush Mehta wrote: > > From: Manish Narani <manish.narani@xilinx.com> > > > > The hibernation feature enabled for Xilinx ZynqMP SoC in DWC3 IP. > > Added the below interrupt-names in the binding schema for the same. > > > > dwc_usb3: dwc3 core interrupt-names > > otg: otg interrupt-names > > hiber: hibernation interrupt-names > > This does not make sense in commit msg. Don't duplicate patch in commit > msg. [Piyush]: Will rephrase the commit message and send V2. > Where is the user (DTS) and implementation of this change? If this is specific > to Xilinx, why you do not have device specific compatible? [Piyush]: We have dedicated irq line for hibernation feature, "hiber" irq line triggers hibernation interrupt. DWC3 core supports the hibernation feature, we have a dedicated code which is yet to be upstreamed. As the hibernation feature provided by dwc3-core, so this will be supported by other SOC/vendors. > > > > > Signed-off-by: Manish Narani <manish.narani@xilinx.com> > > Signed-off-by: Piyush Mehta <piyush.mehta@amd.com> > > --- > > Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml > > b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml > > index 1779d08ba1c0..618fa7bd32be 100644 > > --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml > > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml > > @@ -53,6 +53,8 @@ properties: > > - const: dwc_usb3 > > - items: > > enum: [host, peripheral, otg] > > + - items: > > + enum: [dwc_usb3, otg, hiber] > > > > Best regards, > Krzysztof
On 9/15/22 10:44, Krzysztof Kozlowski wrote: > On 14/09/2022 14:15, Mehta, Piyush wrote: >> >>> Where is the user (DTS) and implementation of this change? If this is specific >>> to Xilinx, why you do not have device specific compatible? >> [Piyush]: >> We have dedicated irq line for hibernation feature, "hiber" irq line triggers hibernation interrupt. >> DWC3 core supports the hibernation feature, we have a dedicated code which is yet to be upstreamed. >> As the hibernation feature provided by dwc3-core, so this will be supported by other SOC/vendors. > > But is hiber irq line present in other vendors? What confuses me is > adding not only "hiber" irq but also otg in completely new enum. I will let Piyush to comment hiber IRQ. But I expect we don't have visibility what others are doing but this is line is not Xilinx invention that's why I expect IP from Synopsys have it by default but it is up to soc vendor if hibernation feature is enabled or not. otg is already listed in Documentation/devicetree/bindings/usb/snps,dwc3.yaml It is only about order. Driver is already using platform_get_irq_byname..() functions I think any combination should be fine. Do we need to record used order or there is way in yaml to support any combination with dwc_usb3, host, peripheral, otg should be working (ignoring that hiber which should be likely there too). Thanks, Michal
On 9/16/22 12:10, Krzysztof Kozlowski wrote: > On 15/09/2022 10:04, Michal Simek wrote: >> >> >> On 9/15/22 10:44, Krzysztof Kozlowski wrote: >>> On 14/09/2022 14:15, Mehta, Piyush wrote: >>>> >>>>> Where is the user (DTS) and implementation of this change? If this is specific >>>>> to Xilinx, why you do not have device specific compatible? >>>> [Piyush]: >>>> We have dedicated irq line for hibernation feature, "hiber" irq line triggers hibernation interrupt. >>>> DWC3 core supports the hibernation feature, we have a dedicated code which is yet to be upstreamed. >>>> As the hibernation feature provided by dwc3-core, so this will be supported by other SOC/vendors. >>> >>> But is hiber irq line present in other vendors? What confuses me is >>> adding not only "hiber" irq but also otg in completely new enum. >> >> I will let Piyush to comment hiber IRQ. But I expect we don't have visibility >> what others are doing but this is line is not Xilinx invention that's why I >> expect IP from Synopsys have it by default but it is up to soc vendor if >> hibernation feature is enabled or not. >> >> otg is already listed in >> Documentation/devicetree/bindings/usb/snps,dwc3.yaml >> >> It is only about order. >> Driver is already using >> platform_get_irq_byname..() functions > > Linux driver yes, but other platforms (bootloaders, operating systems) > might be doing things differently. Therefore the order and items are > usually strict. If they cannot be strict, it is nice to know why or it > is nice to restrict it to some specific variant (if it is applicable). > > This is why I asked whether the line is specific to Xilinx or to others. > >> >> I think any combination should be fine. Do we need to record used order or there >> is way in yaml to support any combination with dwc_usb3, host, peripheral, otg >> should be working (ignoring that hiber which should be likely there too). > > What confuses me here more, is having otg. I understand that dwc_usb3 is > the single interrupt for all the modes, so my naive approach would be: > oneOf: > - dwc_usb3 > - enum [dwc_usb3, hiber] > - enum [host, peripheral, otg] > - enum [host, peripheral, otg, hiber] > > However here Piyush adds not only hiber but also otg... I was looking at code and I think we should be able to use this order - enum [host, peripheral, otg, hiber] which should ensure compatibility in other SW projects. We can completely ignore dwc_usb3. It means above dwc_usb3, hiber shouldn't be also listed to make sure that the second entry is all the time irq for peripheral. Thanks, Michal
On 23/09/2022 06:38, Mehta, Piyush wrote: >> Thanks, >> Michal > > Enabling wakeup in zynqMp we need to put the core into hibernation, as versal don't have hibernation concept, but we require interrupt for wakeup. > We have a versal platform where we are not using hibernation, but system wake up we need the interrupt. For this interrupt-name enum would be: > - enum [host, peripheral, otg, usb-wakeup] > > zynqMp : > - enum [host, peripheral, otg, hiber] > > Versal: > - enum [host, peripheral, otg, usb-wakeup] That's a different name you use now... Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml index 1779d08ba1c0..618fa7bd32be 100644 --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml @@ -53,6 +53,8 @@ properties: - const: dwc_usb3 - items: enum: [host, peripheral, otg] + - items: + enum: [dwc_usb3, otg, hiber] clocks: description: