mbox series

[v4,0/2] Add a separate DWC3 OF driver for Xilinx platforms

Message ID 1615963949-75320-1-git-send-email-manish.narani@xilinx.com
Headers show
Series Add a separate DWC3 OF driver for Xilinx platforms | expand

Message

Manish Narani March 17, 2021, 6:52 a.m. UTC 
This patch series documents the Xilinx Versal DWC3 controller. This also
adds a new Xilinx specific driver for adding new features in the future.

Changes in v2:
	- Addressed review comments from v1
	- merged normal and runtime suspend resume functions as they are
	  same
	- Improved description of some register operations to avoid
	  confusion
	- Updated commit log for patch 2/2 for better clarity.

Changes in v3:
	- Removed snps,enable-hibernation property from the devicetree
	  binding.

Changes in v4:
	- Error checking added for devm_phy_get
	- Documented resets in dt-bindings

Manish Narani (2):
  dt-bindings: usb: dwc3-xilinx: Add documentation for Versal DWC3
    Controller
  usb: dwc3: Add driver for Xilinx platforms

 .../devicetree/bindings/usb/dwc3-xilinx.txt        |  28 +-
 drivers/usb/dwc3/Kconfig                           |   9 +
 drivers/usb/dwc3/Makefile                          |   1 +
 drivers/usb/dwc3/dwc3-of-simple.c                  |   1 -
 drivers/usb/dwc3/dwc3-xilinx.c                     | 339 +++++++++++++++++++++
 5 files changed, 375 insertions(+), 3 deletions(-)
 create mode 100644 drivers/usb/dwc3/dwc3-xilinx.c

Comments

Guenter Roeck April 7, 2021, 9:48 p.m. UTC | #1 
On Wed, Mar 17, 2021 at 12:22:29PM +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.

> 

> Signed-off-by: Manish Narani <manish.narani@xilinx.com>


Trying this driver with qemu (v6.0.0-rc2) results in:

[   15.184242] dwc3-xilinx ff9d0000.usb: error -ENODEV: failed to assert Reset
[   15.185754] Unable to handle kernel paging request at virtual address 006b6b6b6b6b6b9b
[   15.185994] Mem abort info:
[   15.186065]   ESR = 0x96000004
[   15.186317]   EC = 0x25: DABT (current EL), IL = 32 bits
[   15.186414]   SET = 0, FnV = 0
[   15.186498]   EA = 0, S1PTW = 0
[   15.186579] Data abort info:
[   15.186666]   ISV = 0, ISS = 0x00000004
[   15.186756]   CM = 0, WnR = 0
[   15.186887] [006b6b6b6b6b6b9b] address between user and kernel address ranges
[   15.187436] Internal error: Oops: 96000004 [#1] PREEMPT SMP
[   15.187777] Modules linked in:
[   15.188060] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.12.0-rc6-next-20210406-00006-g05407f068fc9-dirty #1
[   15.188265] Hardware name: Xilinx Versal Virtual development board (DT)
[   15.188495] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
[   15.188614] pc : __clk_put+0x24/0x138
[   15.188716] lr : __clk_put+0x24/0x138
[   15.188791] sp : ffff80001326bac0
[   15.188853] x29: ffff80001326bac0 x28: ffff00000644ed00
[   15.188982] x27: ffff00000421ecd0 x26: ffff00000421e810
[   15.189076] x25: ffff00000644f100 x24: 0000000000000000
[   15.189170] x23: ffff8000126a2570 x22: 0000000000000005
[   15.189271] x21: ffff00000644ed00 x20: ffff000006449970
[   15.189367] x19: 6b6b6b6b6b6b6b6b x18: 0000000000000010
[   15.189456] x17: 0000000000000001 x16: 0000000000000000
[   15.189546] x15: ffff000003af0490 x14: 00000000000001b7
[   15.189642] x13: ffff000003af0490 x12: 00000000ffffffea
[   15.189729] x11: ffff8000123b6460 x10: 0000000000000080
[   15.189815] x9 : 00000000676993c6 x8 : 00000000676993c6
[   15.189941] x7 : 000000007d152ab3 x6 : ffff800012768480
[   15.190047] x5 : 0000000000000000 x4 : 000000007f97631e
[   15.190139] x3 : 00000000d5bdf2c2 x2 : 000000000000000b
[   15.190233] x1 : ffff000003af0040 x0 : 0000000000000001
[   15.190432] Call trace:
[   15.190506]  __clk_put+0x24/0x138
[   15.190588]  clk_put+0x10/0x20
[   15.190653]  clk_bulk_put+0x3c/0x60
[   15.190724]  devm_clk_bulk_release+0x1c/0x28
[   15.190806]  release_nodes+0x1c0/0x2b0
[   15.190887]  devres_release_all+0x38/0x60
[   15.190963]  really_probe+0x1e4/0x3a8
[   15.191042]  driver_probe_device+0x64/0xc8
...

because of ...

> +

> +	ret = devm_clk_bulk_get_all(priv_data->dev, &priv_data->clks);

> +	if (ret < 0)

> +		return ret;

> +

...
> +

> +err_clk_put:

> +	clk_bulk_disable_unprepare(priv_data->num_clocks, priv_data->clks);

> +	clk_bulk_put_all(priv_data->num_clocks, priv_data->clks);


clk_bulk_put_all() is not necessary because of devm_clk_bulk_get_all(),
and results in a double free.

> +static int dwc3_xlnx_remove(struct platform_device *pdev)

> +{

> +	struct dwc3_xlnx	*priv_data = platform_get_drvdata(pdev);

> +	struct device		*dev = &pdev->dev;

> +

> +	of_platform_depopulate(dev);

> +

> +	clk_bulk_disable_unprepare(priv_data->num_clocks, priv_data->clks);

> +	clk_bulk_put_all(priv_data->num_clocks, priv_data->clks);


Same here. This will likely crash the driver on unload.

Guenter
Michal Simek April 8, 2021, 6:08 a.m. UTC | #2 
Hi Guenter,

On 4/7/21 11:48 PM, Guenter Roeck wrote:
> On Wed, Mar 17, 2021 at 12:22:29PM +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.

>>

>> Signed-off-by: Manish Narani <manish.narani@xilinx.com>

> 

> Trying this driver with qemu (v6.0.0-rc2) results in:

> 

> [   15.184242] dwc3-xilinx ff9d0000.usb: error -ENODEV: failed to assert Reset

> [   15.185754] Unable to handle kernel paging request at virtual address 006b6b6b6b6b6b9b

> [   15.185994] Mem abort info:

> [   15.186065]   ESR = 0x96000004

> [   15.186317]   EC = 0x25: DABT (current EL), IL = 32 bits

> [   15.186414]   SET = 0, FnV = 0

> [   15.186498]   EA = 0, S1PTW = 0

> [   15.186579] Data abort info:

> [   15.186666]   ISV = 0, ISS = 0x00000004

> [   15.186756]   CM = 0, WnR = 0

> [   15.186887] [006b6b6b6b6b6b9b] address between user and kernel address ranges

> [   15.187436] Internal error: Oops: 96000004 [#1] PREEMPT SMP

> [   15.187777] Modules linked in:

> [   15.188060] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.12.0-rc6-next-20210406-00006-g05407f068fc9-dirty #1

> [   15.188265] Hardware name: Xilinx Versal Virtual development board (DT)

> [   15.188495] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)

> [   15.188614] pc : __clk_put+0x24/0x138

> [   15.188716] lr : __clk_put+0x24/0x138

> [   15.188791] sp : ffff80001326bac0

> [   15.188853] x29: ffff80001326bac0 x28: ffff00000644ed00

> [   15.188982] x27: ffff00000421ecd0 x26: ffff00000421e810

> [   15.189076] x25: ffff00000644f100 x24: 0000000000000000

> [   15.189170] x23: ffff8000126a2570 x22: 0000000000000005

> [   15.189271] x21: ffff00000644ed00 x20: ffff000006449970

> [   15.189367] x19: 6b6b6b6b6b6b6b6b x18: 0000000000000010

> [   15.189456] x17: 0000000000000001 x16: 0000000000000000

> [   15.189546] x15: ffff000003af0490 x14: 00000000000001b7

> [   15.189642] x13: ffff000003af0490 x12: 00000000ffffffea

> [   15.189729] x11: ffff8000123b6460 x10: 0000000000000080

> [   15.189815] x9 : 00000000676993c6 x8 : 00000000676993c6

> [   15.189941] x7 : 000000007d152ab3 x6 : ffff800012768480

> [   15.190047] x5 : 0000000000000000 x4 : 000000007f97631e

> [   15.190139] x3 : 00000000d5bdf2c2 x2 : 000000000000000b

> [   15.190233] x1 : ffff000003af0040 x0 : 0000000000000001

> [   15.190432] Call trace:

> [   15.190506]  __clk_put+0x24/0x138

> [   15.190588]  clk_put+0x10/0x20

> [   15.190653]  clk_bulk_put+0x3c/0x60

> [   15.190724]  devm_clk_bulk_release+0x1c/0x28

> [   15.190806]  release_nodes+0x1c0/0x2b0

> [   15.190887]  devres_release_all+0x38/0x60

> [   15.190963]  really_probe+0x1e4/0x3a8

> [   15.191042]  driver_probe_device+0x64/0xc8

> ...

> 

> because of ...

> 

>> +

>> +	ret = devm_clk_bulk_get_all(priv_data->dev, &priv_data->clks);

>> +	if (ret < 0)

>> +		return ret;

>> +

> ...

>> +

>> +err_clk_put:

>> +	clk_bulk_disable_unprepare(priv_data->num_clocks, priv_data->clks);

>> +	clk_bulk_put_all(priv_data->num_clocks, priv_data->clks);

> 

> clk_bulk_put_all() is not necessary because of devm_clk_bulk_get_all(),

> and results in a double free.

> 

>> +static int dwc3_xlnx_remove(struct platform_device *pdev)

>> +{

>> +	struct dwc3_xlnx	*priv_data = platform_get_drvdata(pdev);

>> +	struct device		*dev = &pdev->dev;

>> +

>> +	of_platform_depopulate(dev);

>> +

>> +	clk_bulk_disable_unprepare(priv_data->num_clocks, priv_data->clks);

>> +	clk_bulk_put_all(priv_data->num_clocks, priv_data->clks);

> 

> Same here. This will likely crash the driver on unload.

It looks like that you directly created the patch. Isn't it better to
send it yourself? Or do you want Manish to create it based on guidance
above?

Manish: Can you please take a look at this?

Thanks,
Michal
Marco Hoefle April 8, 2021, 7:11 p.m. UTC | #3 
Hello Michal, Manish and Guenter,

I recently opened a thread regarding of the PSGTR driver documentation ( 
https://www.spinics.net/lists/devicetree/msg416470.html 
<https://www.spinics.net/lists/devicetree/msg416470.html> )

It is not clear to me how to "marry" DWC3 and PSGTR in the device tree.

There were some significant changes compared to the Xlnx 5.4 Kernel.

You have dwc3 (USB3) working using the mainline Kernel?

Could you please share your device tree?

That would be really helpful to get the mainline kernel running on the 
Ultra96v2.

Thanks

Marco

>

>

>

> On 08.04.21 08:08, Michal Simek wrote:

>> Hi Guenter,

>>

>> On 4/7/21 11:48 PM, Guenter Roeck wrote:

>>> On Wed, Mar 17, 2021 at 12:22:29PM +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.

>>>>

>>>> Signed-off-by: Manish Narani<manish.narani@xilinx.com>

>>> Trying this driver with qemu (v6.0.0-rc2) results in:

>>>

>>> [   15.184242] dwc3-xilinx ff9d0000.usb: error -ENODEV: failed to assert Reset

>>> [   15.185754] Unable to handle kernel paging request at virtual address 006b6b6b6b6b6b9b

>>> [   15.185994] Mem abort info:

>>> [   15.186065]   ESR = 0x96000004

>>> [   15.186317]   EC = 0x25: DABT (current EL), IL = 32 bits

>>> [   15.186414]   SET = 0, FnV = 0

>>> [   15.186498]   EA = 0, S1PTW = 0

>>> [   15.186579] Data abort info:

>>> [   15.186666]   ISV = 0, ISS = 0x00000004

>>> [   15.186756]   CM = 0, WnR = 0

>>> [   15.186887] [006b6b6b6b6b6b9b] address between user and kernel address ranges

>>> [   15.187436] Internal error: Oops: 96000004 [#1] PREEMPT SMP

>>> [   15.187777] Modules linked in:

>>> [   15.188060] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.12.0-rc6-next-20210406-00006-g05407f068fc9-dirty #1

>>> [   15.188265] Hardware name: Xilinx Versal Virtual development board (DT)

>>> [   15.188495] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)

>>> [   15.188614] pc : __clk_put+0x24/0x138

>>> [   15.188716] lr : __clk_put+0x24/0x138

>>> [   15.188791] sp : ffff80001326bac0

>>> [   15.188853] x29: ffff80001326bac0 x28: ffff00000644ed00

>>> [   15.188982] x27: ffff00000421ecd0 x26: ffff00000421e810

>>> [   15.189076] x25: ffff00000644f100 x24: 0000000000000000

>>> [   15.189170] x23: ffff8000126a2570 x22: 0000000000000005

>>> [   15.189271] x21: ffff00000644ed00 x20: ffff000006449970

>>> [   15.189367] x19: 6b6b6b6b6b6b6b6b x18: 0000000000000010

>>> [   15.189456] x17: 0000000000000001 x16: 0000000000000000

>>> [   15.189546] x15: ffff000003af0490 x14: 00000000000001b7

>>> [   15.189642] x13: ffff000003af0490 x12: 00000000ffffffea

>>> [   15.189729] x11: ffff8000123b6460 x10: 0000000000000080

>>> [   15.189815] x9 : 00000000676993c6 x8 : 00000000676993c6

>>> [   15.189941] x7 : 000000007d152ab3 x6 : ffff800012768480

>>> [   15.190047] x5 : 0000000000000000 x4 : 000000007f97631e

>>> [   15.190139] x3 : 00000000d5bdf2c2 x2 : 000000000000000b

>>> [   15.190233] x1 : ffff000003af0040 x0 : 0000000000000001

>>> [   15.190432] Call trace:

>>> [   15.190506]  __clk_put+0x24/0x138

>>> [   15.190588]  clk_put+0x10/0x20

>>> [   15.190653]  clk_bulk_put+0x3c/0x60

>>> [   15.190724]  devm_clk_bulk_release+0x1c/0x28

>>> [   15.190806]  release_nodes+0x1c0/0x2b0

>>> [   15.190887]  devres_release_all+0x38/0x60

>>> [   15.190963]  really_probe+0x1e4/0x3a8

>>> [   15.191042]  driver_probe_device+0x64/0xc8

>>> ...

>>>

>>> because of ...

>>>

>>>> +

>>>> +	ret = devm_clk_bulk_get_all(priv_data->dev, &priv_data->clks);

>>>> +	if (ret < 0)

>>>> +		return ret;

>>>> +

>>> ...

>>>> +

>>>> +err_clk_put:

>>>> +	clk_bulk_disable_unprepare(priv_data->num_clocks, priv_data->clks);

>>>> +	clk_bulk_put_all(priv_data->num_clocks, priv_data->clks);

>>> clk_bulk_put_all() is not necessary because of devm_clk_bulk_get_all(),

>>> and results in a double free.

>>>

>>>> +static int dwc3_xlnx_remove(struct platform_device *pdev)

>>>> +{

>>>> +	struct dwc3_xlnx	*priv_data = platform_get_drvdata(pdev);

>>>> +	struct device		*dev = &pdev->dev;

>>>> +

>>>> +	of_platform_depopulate(dev);

>>>> +

>>>> +	clk_bulk_disable_unprepare(priv_data->num_clocks, priv_data->clks);

>>>> +	clk_bulk_put_all(priv_data->num_clocks, priv_data->clks);

>>> Same here. This will likely crash the driver on unload.

>> It looks like that you directly created the patch. Isn't it better to

>> send it yourself? Or do you want Manish to create it based on guidance

>> above?

>>

>> Manish: Can you please take a look at this?

>>

>> Thanks,

>> Michal

> -- 

> ______________________

>

> Marco Höfle

> Kappelen 11

> CH-⁠5706 Boniswil

> Tel.: +41 78 790 93 62


-- 
______________________

Marco Höfle
Kappelen 11
CH-⁠5706 Boniswil
Tel.: +41 78 790 93 62
Guenter Roeck April 11, 2021, 2:02 p.m. UTC | #4 
Michal,

On 4/7/21 11:08 PM, Michal Simek wrote:
...
> It looks like that you directly created the patch. Isn't it better to

> send it yourself? Or do you want Manish to create it based on guidance

> above?

> 

-next is substantially broken all over the place. I already spend way too much
time bisecting and analyzing the failures, and making sure that the problems
are not caused by qemu (which is why I tracked down this problem in such detail).
I don't really have time to write patches and guide them through the process,
sorry.

Guenter
Michal Simek April 12, 2021, 8:01 a.m. UTC | #5 
Hi Guenter,

On 4/11/21 4:02 PM, Guenter Roeck wrote:
> Michal,

> 

> On 4/7/21 11:08 PM, Michal Simek wrote:

> ...

>> It looks like that you directly created the patch. Isn't it better to

>> send it yourself? Or do you want Manish to create it based on guidance

>> above?

>>

> -next is substantially broken all over the place. I already spend way too much

> time bisecting and analyzing the failures, and making sure that the problems

> are not caused by qemu (which is why I tracked down this problem in such detail).

> I don't really have time to write patches and guide them through the process,

> sorry.


I definitely appreciate your help on this.

Manish has sent a patch for it already.
http://lore.kernel.org/r/1617904448-74611-3-git-send-email-manish.narani@xilinx.com

Not sure why he hasn't added you to CC but at least Reported-by: tag
with your name should be added.

Thanks,
Michal