RE: [RESEND PATCH v3 2/2] usb: dwc3: Add driver for Xilinx platforms

[Manish Narani]

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