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

[Michael Grzeschik <mgr@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 9673 bytes --]

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 <mgr@pengutronix.de>
>>>Sent: Tuesday, February 9, 2021 5:26 AM
>>>To: Manish Narani <MNARANI@xilinx.com>
>>>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
>>><michals@xilinx.com>; git <git@xilinx.com>; 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 <mgr@pengutronix.de>
>>>>>>>Sent: Friday, January 22, 2021 1:39 PM
>>>>>>>To: Manish Narani <MNARANI@xilinx.com>
>>>>>>>Cc: devicetree@vger.kernel.org; kernel@pengutronix.de;
>>>balbi@kernel.org;
>>>>>>>gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; Michal Simek
>>>>>>><michals@xilinx.com>; linux-kernel@vger.kernel.org;
>>>robh+dt@kernel.org;
>>>>>>>git <git@xilinx.com>; 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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]