Adding Tinh Nguyen to CC: On Tue, Feb 09, 2021 at 09:02:00PM +0100, Michael Grzeschik wrote: >Hi Manish, > >On Tue, Feb 09, 2021 at 06:01:58AM +0000, Manish Narani wrote: >>Hi Michael, >> >>>-----Original Message----- >>>From: Michael Grzeschik >>>Sent: Tuesday, February 9, 2021 5:26 AM >>>To: Manish Narani >>>Cc: devicetree@vger.kernel.org; p.zabel@pengutronix.de; balbi@kernel.org; >>>gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; linux- >>>kernel@vger.kernel.org; robh+dt@kernel.org; Michal Simek >>>; git ; kernel@pengutronix.de; linux- >>>arm-kernel@lists.infradead.org >>>Subject: Re: [RESEND PATCH v3 2/2] usb: dwc3: Add driver for Xilinx >>>platforms >>> >>>Hi Manish! >>> >>>On Thu, Jan 28, 2021 at 12:36:07AM +0100, Michael Grzeschik wrote: >>>>On Fri, Jan 22, 2021 at 02:34:52PM +0100, Michael Grzeschik wrote: >>>>>On Fri, Jan 22, 2021 at 01:06:22PM +0000, Manish Narani wrote: >>>>>>Hi Michael, >>>>>> >>>>>>>-----Original Message----- >>>>>>>From: Michael Grzeschik >>>>>>>Sent: Friday, January 22, 2021 1:39 PM >>>>>>>To: Manish Narani >>>>>>>Cc: devicetree@vger.kernel.org; kernel@pengutronix.de; >>>balbi@kernel.org; >>>>>>>gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; Michal Simek >>>>>>>; linux-kernel@vger.kernel.org; >>>robh+dt@kernel.org; >>>>>>>git ; p.zabel@pengutronix.de; linux-arm- >>>>>>>kernel@lists.infradead.org >>>>>>>Subject: Re: [RESEND PATCH v3 2/2] usb: dwc3: Add driver for Xilinx >>>>>>>platforms >>>>>>> >>>>>>>Hello! >>>>>>> >>>>>>>On Mon, Jan 18, 2021 at 02:42:24PM +0100, Michael Grzeschik wrote: >>>>>>>>Hi! >>>>>>>> >>>>>>>>On Tue, Dec 15, 2020 at 12:24:51PM +0530, Manish Narani wrote: >>>>>>>>>Add a new driver for supporting Xilinx platforms. This driver is used >>>>>>>>>for some sequence of operations required for Xilinx USB controllers. >>>>>>>>>This driver is also used to choose between PIPE clock coming from >>>SerDes >>>>>>>>>and the Suspend Clock. Before the controller is out of reset, the clock >>>>>>>>>selection should be changed to PIPE clock in order to make the USB >>>>>>>>>controller work. There is a register added in Xilinx USB controller >>>>>>>>>register space for the same. >>>>>>>> >>>>>>>>I tried out this driver with the vanilla kernel on an zynqmp. Without >>>>>>>>this patch the USB-Gadget is already acting buggy. In the gadget mode, >>>>>>>>some iterations of plug/unplug results to an stalled gadget which will >>>>>>>>never come back without a reboot. >>>>>>>> >>>>>>>>With the corresponding code of this driver (reset assert, clk modify, >>>>>>>>reset deassert) in the downstream kernels phy driver we found out it is >>>>>>>>totaly stable. But using this exact glue driver which should do the same >>>>>>>>as the downstream code, the gadget still was buggy the way described >>>>>>>>above. >>>>>>>> >>>>>>>>I suspect the difference lays in the different order of operations. >>>>>>>>While the downstream code is runing the resets inside the phy driver >>>>>>>>which is powered and initialized in the dwc3-core itself. With this glue >>>>>>>>layser approach of this patch the whole phy init is done before even >>>>>>>>touching dwc3-core in any way. It seems not to have the same effect, >>>>>>>>though. >>>>>>>> >>>>>>>>If really the order of operations is limiting us, we probably need >>>>>>>>another solution than this glue layer. Any Ideas? >>>>>>> >>>>>>>I found out what the difference between the Downstream and this >>>>>>>Glue is. When using vanilla with this Glue code we may not set >>>>>>>the following bit: >>>>>>> >>>>>>>https://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq- >>>>>>>ultrascale-registers.html#usb3_regs___fpd_power_prsnt.html >>>>>>> >>>>>>>>>+ /* Set PIPE Power Present signal in FPD Power Present >>>Register*/ >>>>>>>>>+ writel(PIPE_POWER_ON, priv_data->regs + >>>>>>>XLNX_USB_FPD_POWER_PRSNT); >>>>>>> >>>>>>>When I comment this out, the link stays stable. This is different in >>>>>>>the Downstream Xilinx Kernel, where the bit is also set but has no >>>>>>>negativ effect. >>>>>>> >>>>>>>Manish, can you give me a pointer what to look for? >>>>>>>So setting this will also work with mainline? >>>>>>I am looking further on this but from what I see here is that, >>>>>>In order to make USB function properly, there are some dt changes >>>needed in mainline for >>>>>>USB node which include defining clocks coming from serdes. >>>>>>The DT changes are pending to be sent to mainline. >>>>> >>>>>Can you push that state somewhere, so I could test it? >>>>>Or is in the downstream kernel some things to copy? >>>>> >>>>>>Can you share the DT settings for USB node on your side? >>>>> >>>>>Here is my current configuration for the device node at usb0: >>>>> >>>>>zynqmp.dtsi >>>>> >>>>>zynqmp_reset: reset-controller { >>>>> compatible = "xlnx,zynqmp-reset"; >>>>> #reset-cells = <1>; >>>>>}; >>>>> >>>>>usb0: usb@ff9d0000 { >>>>> #address-cells = <2>; >>>>> #size-cells = <2>; >>>>> status = "disabled"; >>>>> compatible = "xlnx,zynqmp-dwc3"; >>>>> reg = <0x0 0xff9d0000 0x0 0x100>; >>>>> clock-names = "bus_clk", "ref_clk"; >>>>> power-domains = <&zynqmp_firmware PD_USB_0>; >>>>> ranges; >>>>> resets = <&zynqmp_reset ZYNQMP_RESET_USB0_CORERESET>, >>>>> <&zynqmp_reset ZYNQMP_RESET_USB0_HIBERRESET>, >>>>> <&zynqmp_reset ZYNQMP_RESET_USB0_APB>; >>>>> reset-names = "usb_crst", "usb_hibrst", "usb_apbrst"; >>>>> phy-names = "usb3-phy"; >>>>> phys = <&psgtr 2 PHY_TYPE_USB3 0 2>; >>>>> >>>>> usb0_dwc3: dwc3@fe200000 { >>>>> compatible = "snps,dwc3"; >>>>> interrupt-parent = <&gic>; >>>>> interrupts = <0 65 4>; >>>>> clock-names = "ref", "bus_early", "suspend"; >>>>> reg = <0x0 0xfe200000 0x0 0x40000>; >>>>> }; >>>>>}; >>>>> >>>>>platform.dts >>>>> >>>>>&usb0 { >>>>> status = "okay"; >>>>> phy-names = "usb3-phy"; >>>>> phys = <&psgtr 2 PHY_TYPE_USB3 0 2>; >>>>>}; >>>>> >>>>>&usb0_dwc3 { >>>>> dr_mode = "peripheral"; >>>>> >>>>> /* The following quirks are required, since the bInterval is 1 and we >>>>> * handle steady ISOC streaming. See Usecase 3 in commit >>>729dcffd1ed3 >>>>> * ("usb: dwc3: gadget: Add support for disabling U1 and U2 >>>entries"). >>>>> */ >>>>> snps,dis-u1-entry-quirk; >>>>> snps,dis-u2-entry-quirk; >>>>>}; >>>>> >>>>> >>>>>>Meanwhile I will keep updating on the same. >>>>> >>>>>Thanks, that sounds great! >>>> >>>>I have more feedback regarding this issues. As we saw new uncommon >>>>effects, when using the glue. Regarding to get the plug/unplug behaviour >>>>stable, we sticked with leaving out the setting of PIPE_POWER_ON in that >>>>driver. Unfortunately, with that change, the dwc3 is not only not >>>>sending any Erratic Errors any more, but also is lacking to send >>>>disconnect interrupts. >>>> >>>>Double checking with downstream shows that disconnects are working >>>>completely fine in your downstream stack. >>>> >>>>I think we should really need to know why PIPE_POWER_ON is making >>>>a difference before we can say the dwc3 is stable with that glue. >>> >>>After bisecting your v5.4 and mainline we found out that this all is >>>working fine, when setting "snps,dis_u3_susphy_quirk" in the zynqmp.dtsi >>>dwc3 node. >>> >>>The code handling this quirk was introduced after v5.4, so this was >>>never an issue with your downstream stack. >>> >>>"9ba3aca8 usb: dwc3: Disable phy suspend after power-on reset" >>> >>>We need to know if adding snps,dis_u3_susphy_quirk to the dwc nodes >>>is generally necessary for zynqmp, so we can fix for everyone. >> >>Yes, it is necessary for DWC3 on ZynqMP platform. This property should be >>added to the DT node. > >For now this quirk does solve the issues regarding the pluging >behaviour. But we would like to know why? > >Is the phy not properly configured/connected to serve the phy >suspend as intended for the dwc3 stack? Is this a real Hardware issue, >or does this quirk only disable the suspend behaviour even though it >would work properly when configured correctly in software. > > >The second question is addressing the dwc3 xilinx glue driver >you are trying to mainline. We found that the driver is pulling and >releasing some resets before and after changing the pll frequency >comming from the reference clk lines. After that the undocumented >registers XLNX_USB_FPD_POWER_PRSNT and XLNX_USB_FPD_PIPE_CLK are changed >according to the updated pll frequency. What do these bit change? >Is this an internal configuration every dwc3 user on the zynqmp has >to do or does this differ from user to user? > >Regards, >Michael > >-- >Pengutronix e.K. | | >Steuerwalder Str. 21 | http://www.pengutronix.de/ | >31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | >Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | >_______________________________________________ >linux-arm-kernel mailing list >linux-arm-kernel@lists.infradead.org >http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |