RE: [PATCH v2 1/2] dt-bindings: usb: dwc3-xilinx: Add documentation for Versal DWC3 Controller

[Manish Narani]

Hi Felipe,

> -----Original Message-----
> From: Felipe Balbi <balbi@xxxxxxxxxx>
> Sent: Friday, September 25, 2020 12:42 PM
> To: Manish Narani <MNARANI@xxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; Michal Simek <michals@xxxxxxxxxx>;
> p.zabel@xxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; git <git@xxxxxxxxxx>
> Subject: RE: [PATCH v2 1/2] dt-bindings: usb: dwc3-xilinx: Add
> documentation for Versal DWC3 Controller
> 
> 
> Hi,
> 
> Manish Narani <MNARANI@xxxxxxxxxx> writes:
> > Hi Rob/Felipe,
> >
> > Thanks for the review.
> >
> >> -----Original Message-----
> >> From: Felipe Balbi <balbi@xxxxxxxxxx>
> >> Sent: Thursday, September 24, 2020 12:47 PM
> >> To: Rob Herring <robh@xxxxxxxxxx>; Manish Narani
> <MNARANI@xxxxxxxxxx>
> >> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; Michal Simek <michals@xxxxxxxxxx>;
> >> p.zabel@xxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx;
> >> devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> >> kernel@xxxxxxxxxxxxxxx; git <git@xxxxxxxxxx>
> >> Subject: Re: [PATCH v2 1/2] dt-bindings: usb: dwc3-xilinx: Add
> >> documentation for Versal DWC3 Controller
> >>
> >> Rob Herring <robh@xxxxxxxxxx> writes:
> >>
> >> > On Thu, Sep 10, 2020 at 12:33:04AM +0530, Manish Narani wrote:
> >> >> Add documentation for Versal DWC3 controller. Add required property
> >> >> 'reg' for the same. Also add optional properties for snps,dwc3.
> >> >>
> >> >> Signed-off-by: Manish Narani <manish.narani@xxxxxxxxxx>
> >> >> ---
> >> >>  .../devicetree/bindings/usb/dwc3-xilinx.txt   | 20 +++++++++++++++++-
> -
> >> >>  1 file changed, 18 insertions(+), 2 deletions(-)
> >> >>
> >> >> diff --git a/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
> >> b/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
> >> >> index 4aae5b2cef56..219b5780dbee 100644
> >> >> --- a/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
> >> >> +++ b/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
> >> >> @@ -1,7 +1,8 @@
> >> >>  Xilinx SuperSpeed DWC3 USB SoC controller
> >> >>
> >> >>  Required properties:
> >> >> -- compatible:	Should contain "xlnx,zynqmp-dwc3"
> >> >> +- compatible:	May contain "xlnx,zynqmp-dwc3" or "xlnx,versal-
> >> dwc3"
> >> >> +- reg:		Base address and length of the register control block
> >> >>  - clocks:	A list of phandles for the clocks listed in clock-names
> >> >>  - clock-names:	Should contain the following:
> >> >>    "bus_clk"	 Master/Core clock, have to be >= 125 MHz for SS
> >> >> @@ -13,12 +14,24 @@ Required child node:
> >> >>  A child node must exist to represent the core DWC3 IP block. The
> name of
> >> >>  the node is not important. The content of the node is defined in
> dwc3.txt.
> >> >>
> >> >> +Optional properties for snps,dwc3:
> >> >> +- dma-coherent:	Enable this flag if CCI is enabled in design. Adding this
> >> >> +		flag configures Global SoC bus Configuration Register and
> >> >> +		Xilinx USB 3.0 IP - USB coherency register to enable CCI.
> >> >> +- snps,enable-hibernation: Add this flag to enable hibernation support
> >> for
> >> >> +		peripheral mode.
> >> >
> >> > This belongs in the DWC3 binding. It also implies that hibernation is
> >> > not supported by any other DWC3 based platform. Can't this be implied
> by
> >> > the compatible string (in the parent)?
> >
> > Rob, We can move this to dwc3 bindings. If Felipe is okay with below
> response.
> >
> >>
> >> hibernation support is detectable in runtime, and we've been using that.
> >
> > Felipe, Yes, this flag is to control the enable/disable hibernation.
> > I did not see has_hibernation flag being set anywhere in the driver.
> > Can we control the hibernation enable/disable through DT entry? See
> below:
> > -----
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 2eb34c8b4065..1baf44d8d566 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -769,8 +769,15 @@ static void dwc3_core_setup_global_control(struct
> dwc3 *dwc)
> >                         reg &= ~DWC3_GCTL_DSBLCLKGTNG;
> >                 break;
> >         case DWC3_GHWPARAMS1_EN_PWROPT_HIB:
> > -               /* enable hibernation here */
> > -               dwc->nr_scratch =
> DWC3_GHWPARAMS4_HIBER_SCRATCHBUFS(hwparams4);
> > +               if (!device_property_read_bool(dwc->dev,
> > +                                              "snps,enable-hibernation")) {
> > +                       dev_dbg(dwc->dev, "Hibernation not enabled\n");
> > +               } else {
> > +                       /* enable hibernation here */
> > +                       dwc->nr_scratch =
> > +
> DWC3_GHWPARAMS4_HIBER_SCRATCHBUFS(hwparams4);
> > +                       dwc->has_hibernation = 1;
> > +               }
> 
> I left it off because I didn't have HW to validate. Don't add a new
> binding for this. Set has_hibernation to true and make sure it
> works. Then send me a patch that sets has_hibernation to true whenever
> DWC3_GHWPARAMS1_EN_PWROPT_HIB is valid.

OK Felipe. I will remove this property from binding. We have validated
Device-mode hibernation on Xilinx ZynqMP and Versal platform. I am
planning to send a separate patch series for hibernation after this.

Thanks,
Manish