Hi Michael, > -----Original Message----- > From: Michael Grzeschik <mgr@xxxxxxxxxxxxxx> > Sent: Tuesday, February 9, 2021 5:26 AM > To: Manish Narani <MNARANI@xxxxxxxxxx> > Cc: devicetree@xxxxxxxxxxxxxxx; p.zabel@xxxxxxxxxxxxxx; balbi@xxxxxxxxxx; > gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx; Michal Simek > <michals@xxxxxxxxxx>; git <git@xxxxxxxxxx>; kernel@xxxxxxxxxxxxxx; linux- > arm-kernel@xxxxxxxxxxxxxxxxxxx > 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 <mgr@xxxxxxxxxxxxxx> > >>>>Sent: Friday, January 22, 2021 1:39 PM > >>>>To: Manish Narani <MNARANI@xxxxxxxxxx> > >>>>Cc: devicetree@xxxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; > balbi@xxxxxxxxxx; > >>>>gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; Michal Simek > >>>><michals@xxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; > robh+dt@xxxxxxxxxx; > >>>>git <git@xxxxxxxxxx>; p.zabel@xxxxxxxxxxxxxx; linux-arm- > >>>>kernel@xxxxxxxxxxxxxxxxxxx > >>>>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. Thanks, Manish
