ARM: dts: exynos: update the usbdrd phy and ref clk
Subsystems (3)
RFC 1/2 ARM: dts: exynos: update the usbdrd phy and ref clk
update the usbdrd link control and phy contol clks.
---
Tested on Odroid XU4 and Odroid HC1 develpment board. Did not test Odroid XU develpment board.
--- arch/arm/boot/dts/exynos5410.dtsi | 4 ++-- arch/arm/boot/dts/exynos5420.dtsi | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
arch/arm/boot/dts/exynos5410.dtsi +2 -2
@@ -385,7 +385,7 @@ }; &usbdrd_phy0 { - clocks = <&clock CLK_USBD300>, <&clock CLK_SCLK_USBPHY300>; + clocks = <&clock CLK_SCLK_USBPHY300>, <&clock CLK_SCLK_USBD300>; clock-names = "phy", "ref"; samsung,pmu-syscon = <&pmu_system_controller>; };
@@ -400,7 +400,7 @@ }; &usbdrd_phy1 { - clocks = <&clock CLK_USBD301>, <&clock CLK_SCLK_USBPHY301>; + clocks = <&clock CLK_SCLK_USBPHY301>, <&clock CLK_SCLK_USBD301>; clock-names = "phy", "ref"; samsung,pmu-syscon = <&pmu_system_controller>; };
arch/arm/boot/dts/exynos5420.dtsi +2 -2
@@ -1461,7 +1461,7 @@ }; &usbdrd_phy0 { - clocks = <&clock CLK_USBD300>, <&clock CLK_SCLK_USBPHY300>; + clocks = <&clock CLK_SCLK_USBPHY300>, <&clock CLK_SCLK_USBD300>; clock-names = "phy", "ref"; samsung,pmu-syscon = <&pmu_system_controller>; };
@@ -1476,7 +1476,7 @@ }; &usbdrd_phy1 { - clocks = <&clock CLK_USBD301>, <&clock CLK_SCLK_USBPHY301>; + clocks = <&clock CLK_SCLK_USBPHY301>, <&clock CLK_SCLK_USBD301>; clock-names = "phy", "ref"; samsung,pmu-syscon = <&pmu_system_controller>; };
Signature · 1 line
2.7.4
RFC 2/2 phy: exynos5-usbdrd: remove disable and enable of phy clk
remove the disable and enable of phy clk.
phy clk is needed to tune the phy controller.
Before:
mout_usbd300 1 1 24000000 0 0
dout_usbd300 0 0 24000000 0 0
sclk_usbd300 0 0 24000000 0 0
dout_usbphy300 1 1 24000000 0 0
sclk_usbphy300 3 3 24000000 0 0
mout_usbd301 1 1 24000000 0 0
dout_usbd301 0 0 24000000 0 0
sclk_usbd301 0 0 24000000 0 0
dout_usbphy301 1 1 24000000 0 0
sclk_usbphy301 2 2 24000000 0 0
After:
mout_usbd300 2 2 24000000 0 0
dout_usbd300 1 1 24000000 0 0
sclk_usbd300 2 2 24000000 0 0
dout_usbphy300 1 1 24000000 0 0
sclk_usbphy300 3 3 24000000 0 0
mout_usbd301 2 2 24000000 0 0
dout_usbd301 1 1 24000000 0 0
sclk_usbd301 2 2 24000000 0 0
dout_usbphy301 1 1 24000000 0 0
sclk_usbphy301 2 2 24000000 0 0--- drivers/phy/samsung/phy-exynos5-usbdrd.c | 7 ------- 1 file changed, 7 deletions(-)
drivers/phy/samsung/phy-exynos5-usbdrd.c +0 -7
@@ -409,22 +409,15 @@ static int exynos5_usbdrd_phy_init(struct phy *phy) reg &= ~PHYCLKRST_PORTRESET ;writel (reg, phy_drd->reg_phy +EXYNOS5_DRD_PHYCLKRST ); -clk_disable_unprepare (phy_drd->clk); -return 0 ; }static int exynos5_usbdrd_phy_exit (struct phy *phy) { -int ret;u32 reg;struct phy_usb_instance *inst =phy_get_drvdata (phy);struct exynos5_usbdrd_phy *phy_drd =to_usbdrd_phy (inst); - ret =clk_prepare_enable (phy_drd->clk); -if (ret) -return ret; - reg =PHYUTMI_OTGDISABLE |PHYUTMI_FORCESUSPEND |PHYUTMI_FORCESLEEP ;
Signature · 1 line
2.7.4
Krzysztof Kozlowski
maintainerRe: [RFC 2/2] phy: exynos5-usbdrd: remove disable and enable of phy clk
Quoted 2 lines · Anand Moon
remove the disable and enable of phy clk.
phy clk is needed to tune the phy controller.
Drivers should in general enable and disable the clocks they use. Just like in patch #1 please describe why you are doing this, what kind of problem are you trying to solve and what exactly are you trying to do here.
BR,
Krzysztof
Quoted 59 lines
Before:
mout_usbd300 1 1 24000000 0 0 dout_usbd300 0 0 24000000 0 0 sclk_usbd300 0 0 24000000 0 0 dout_usbphy300 1 1 24000000 0 0 sclk_usbphy300 3 3 24000000 0 0 mout_usbd301 1 1 24000000 0 0 dout_usbd301 0 0 24000000 0 0 sclk_usbd301 0 0 24000000 0 0 dout_usbphy301 1 1 24000000 0 0 sclk_usbphy301 2 2 24000000 0 0 After: mout_usbd300 2 2 24000000 0 0 dout_usbd300 1 1 24000000 0 0 sclk_usbd300 2 2 24000000 0 0 dout_usbphy300 1 1 24000000 0 0 sclk_usbphy300 3 3 24000000 0 0 mout_usbd301 2 2 24000000 0 0 dout_usbd301 1 1 24000000 0 0 sclk_usbd301 2 2 24000000 0 0 dout_usbphy301 1 1 24000000 0 0 sclk_usbphy301 2 2 24000000 0 0Signed-off-by Anand Moon <[email protected]>--- drivers/phy/samsung/phy-exynos5-usbdrd.c | 7 ------- 1 file changed, 7 deletions(-)drivers/phy/samsung/phy-exynos5-usbdrd.c +0
-8@@ -409,22 +409,15 @@ static int exynos5_usbdrd_phy_init(struct phy *phy) reg &= ~PHYCLKRST_PORTRESET ;writel (reg, phy_drd->reg_phy +EXYNOS5_DRD_PHYCLKRST ); -clk_disable_unprepare (phy_drd->clk); -return 0 ; }static int exynos5_usbdrd_phy_exit (struct phy *phy) { -int ret;u32 reg;struct phy_usb_instance *inst =phy_get_drvdata (phy);struct exynos5_usbdrd_phy *phy_drd =to_usbdrd_phy (inst); - ret =clk_prepare_enable (phy_drd->clk); -if (ret) -return ret; - reg =PHYUTMI_OTGDISABLE |PHYUTMI_FORCESUSPEND |PHYUTMI_FORCESLEEP ; --2.7.4
Re: [RFC 2/2] phy: exynos5-usbdrd: remove disable and enable of phy clk
Hi Krzysztof,
Quoted 4 lines · Krzysztof Kozlowski
Quoted 2 lines · Anand Moon
remove the disable and enable of phy clk.
phy clk is needed to tune the phy controller.
Quoted 8 lines
Drivers should in general enable and disable the clocks they use. Just like in patch #1 please describe why you are doing this, what kind of problem are you trying to solve and what exactly are you trying to do here.
BR,
Krzysztof
[snip]
Usually we would disable the clk on error patch and return with failed. but in the current code we disable the clk in init routine and enable the clk in disable routine which seem incorrect.
[0] https://github.com/torvalds/linux/blob/master/drivers/phy/samsung/phy-exynos5-usbdrd.c#L362-L412
[1] https://github.com/torvalds/linux/blob/master/drivers/phy/samsung/phy-exynos5-usbdrd.c#L424-L446
On 3.10.x kernel clk_summary all the clk are enables.
mout_usbdrd300 3 3 24000000
dout_usbdrd300 2 2 24000000
sclk_usbdrd300 1 1 24000000
dout_usbphy300 2 2 24000000
sclk_usbphy300 1 1 24000000
mout_usbdrd301 3 3 24000000
dout_usbdrd301 2 2 24000000
sclk_usbdrd301 1 1 24000000
dout_usbphy301 2 2 24000000
sclk_usbphy301 1 1 24000000Some more changes in clk driver might be required to stabilize the usb module.
Best Regards
-Anand
Krzysztof Kozlowski
maintainerRe: [RFC 2/2] phy: exynos5-usbdrd: remove disable and enable of phy clk
Quoted 16 lines · Anand Moon
Hi Krzysztof,
Quoted 4 lines · Krzysztof Kozlowski
Quoted 2 lines · Anand Moon
remove the disable and enable of phy clk.
phy clk is needed to tune the phy controller.Quoted 8 lines
Drivers should in general enable and disable the clocks they use. Just like in patch #1 please describe why you are doing this, what kind of problem are you trying to solve and what exactly are you trying to do here.
BR,
Krzysztof
Quoted 5 lines
[snip]
Usually we would disable the clk on error patch and return with failed. but in the current code we disable the clk in init routine and enable the clk in disable routine which seem incorrect.
No. For example for exynos5_usbdrd_phy_exit() the driver enables the clock only for the access to PHY block (PHY registers).
Quoted 17 lines
[0] https://github.com/torvalds/linux/blob/master/drivers/phy/samsung/phy-exynos5-usbdrd.c#L362-L412
[1] https://github.com/torvalds/linux/blob/master/drivers/phy/samsung/phy-exynos5-usbdrd.c#L424-L446On 3.10.x kernel
clk_summaryall the clk are enables.mout_usbdrd300 3 3 24000000 dout_usbdrd300 2 2 24000000 sclk_usbdrd300 1 1 24000000 dout_usbphy300 2 2 24000000 sclk_usbphy300 1 1 24000000 mout_usbdrd301 3 3 24000000 dout_usbdrd301 2 2 24000000 sclk_usbdrd301 1 1 24000000 dout_usbphy301 2 2 24000000 sclk_usbphy301 1 1 24000000
Quoted 1 line
Some more changes in clk driver might be required to stabilize the usb module.
Please describe the issues with stability first.
Best regards,
Krzysztof
Re: [RFC 2/2] phy: exynos5-usbdrd: remove disable and enable of phy clk
Hi Krzysztof,
Quoted 17 lines · Krzysztof Kozlowski
Quoted 16 lines · Anand Moon
Hi Krzysztof,
Quoted 12 lines · Krzysztof Kozlowski
Quoted 2 lines · Anand Moon
remove the disable and enable of phy clk.
phy clk is needed to tune the phy controller.Drivers should in general enable and disable the clocks they use. Just like in patch #1 please describe why you are doing this, what kind of problem are you trying to solve and what exactly are you trying to do here.
BR,
Krzysztof
Quoted 9 lines
Quoted 5 lines
[snip]
Usually we would disable the clk on error patch and return with failed. but in the current code we disable the clk in init routine and enable the clk in disable routine which seem incorrect.
No. For example for
exynos5_usbdrd_phy_exit()the driver enables the clock only for the access to PHY block (PHY registers).
But I dont get it why we disable code phy clk, which could be used in future phy tune or link control as it part of CLK_FSYS.
Quoted 17 lines
Quoted 17 lines
[0] https://github.com/torvalds/linux/blob/master/drivers/phy/samsung/phy-exynos5-usbdrd.c#L362-L412
[1] https://github.com/torvalds/linux/blob/master/drivers/phy/samsung/phy-exynos5-usbdrd.c#L424-L446On 3.10.x kernel
clk_summaryall the clk are enables.mout_usbdrd300 3 3 24000000 dout_usbdrd300 2 2 24000000 sclk_usbdrd300 1 1 24000000 dout_usbphy300 2 2 24000000 sclk_usbphy300 1 1 24000000 mout_usbdrd301 3 3 24000000 dout_usbdrd301 2 2 24000000 sclk_usbdrd301 1 1 24000000 dout_usbphy301 2 2 24000000 sclk_usbphy301 1 1 24000000
Quoted 7 lines
Quoted 1 line
Some more changes in clk driver might be required to stabilize the usb module.
Please describe the issues with stability first.
Best regards,
Krzysztof
On stability:
USB 3.0 read / write performance is not up to mark with moderate data read / write speed,
If you give long compilation of kernel or any source code it would likely be slow
It work pretty good with USB 3.0 SSD but generally failed with 2.5' inc HDD.
If needed I will share you some performance test result with and without these patches.
I have some more changes related to usbdrd phy change queued along this on.
Best Regards
-Anand
Re: [RFC 2/2] phy: exynos5-usbdrd: remove disable and enable of phy clk
hi Krzysztof,
Quoted 26 lines · Anand Moon
Hi Krzysztof,
Quoted 23 lines · Krzysztof Kozlowski
Quoted 21 lines · Anand Moon
Hi Krzysztof,
Quoted 4 lines · Krzysztof Kozlowski
Quoted 2 lines · Anand Moon
remove the disable and enable of phy clk.
phy clk is needed to tune the phy controller.Quoted 8 lines
Drivers should in general enable and disable the clocks they use. Just like in patch #1 please describe why you are doing this, what kind of problem are you trying to solve and what exactly are you trying to do here.
BR,
Krzysztof[snip]
Usually we would disable the clk on error patch and return with failed. but in the current code we disable the clk in init routine and enable the clk in disable routine which seem incorrect.
disable of clk could affect the PMU used to control the phy driver.
Quoted 46 lines
Quoted 3 lines
No. For example for
exynos5_usbdrd_phy_exit()the driver enables the clock only for the access to PHY block (PHY registers).But I dont get it why we disable code phy clk, which could be used in future phy tune or link control as it part of
CLK_FSYS.Quoted 17 lines
Quoted 17 lines
[0] https://github.com/torvalds/linux/blob/master/drivers/phy/samsung/phy-exynos5-usbdrd.c#L362-L412
[1] https://github.com/torvalds/linux/blob/master/drivers/phy/samsung/phy-exynos5-usbdrd.c#L424-L446On 3.10.x kernel
clk_summaryall the clk are enables.mout_usbdrd300 3 3 24000000 dout_usbdrd300 2 2 24000000 sclk_usbdrd300 1 1 24000000 dout_usbphy300 2 2 24000000 sclk_usbphy300 1 1 24000000 mout_usbdrd301 3 3 24000000 dout_usbdrd301 2 2 24000000 sclk_usbdrd301 1 1 24000000 dout_usbphy301 2 2 24000000 sclk_usbphy301 1 1 24000000Quoted 7 lines
Quoted 1 line
Some more changes in clk driver might be required to stabilize the usb module.
Please describe the issues with stability first.
Best regards,
KrzysztofOn stability:
USB 3.0 read / write performance is not up to mark with moderate data read / write speed,
If you give long compilation of kernel or any source code it would likely be slow
It work pretty good with USB 3.0 SSD but generally failed with 2.5' inc HDD.If needed I will share you some performance test result with and without these patches.
I have some more changes related to usbdrd phy change queued along this on.
Best Regards
-Anand
Krzysztof Kozlowski
maintainerRe: [RFC 2/2] phy: exynos5-usbdrd: remove disable and enable of phy clk
Quoted 23 lines · Anand Moon
hi Krzysztof,
Quoted 20 lines · Anand Moon
Hi Krzysztof,
Quoted 17 lines · Krzysztof Kozlowski
Quoted 16 lines · Anand Moon
Hi Krzysztof,
Quoted 12 lines · Krzysztof Kozlowski
Quoted 2 lines · Anand Moon
remove the disable and enable of phy clk.
phy clk is needed to tune the phy controller.Drivers should in general enable and disable the clocks they use. Just like in patch #1 please describe why you are doing this, what kind of problem are you trying to solve and what exactly are you trying to do here.
BR,
Krzysztof
Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
[snip]
Usually we would disable the clk on error patch and return with failed. but in the current code we disable the clk in init routine and enable the clk in disable routine which seem incorrect.
disable of clk could affect the PMU used to control the phy driver.
Disabling device's clock affects the device but the PMU rather not... but I might misunderstood you cause this sounds quite unprecise.
Best regards,
Krzysztof
Krzysztof Kozlowski
maintainerRe: [RFC 2/2] phy: exynos5-usbdrd: remove disable and enable of phy clk
In reply to Anand MoonQuoted 26 lines · Anand Moon
Hi Krzysztof,
Quoted 23 lines · Krzysztof Kozlowski
Quoted 21 lines · Anand Moon
Hi Krzysztof,
Quoted 4 lines · Krzysztof Kozlowski
Quoted 2 lines · Anand Moon
remove the disable and enable of phy clk.
phy clk is needed to tune the phy controller.Quoted 8 lines
Drivers should in general enable and disable the clocks they use. Just like in patch #1 please describe why you are doing this, what kind of problem are you trying to solve and what exactly are you trying to do here.
BR,
Krzysztof[snip]
Usually we would disable the clk on error patch and return with failed. but in the current code we disable the clk in init routine and enable the clk in disable routine which seem incorrect.
Quoted 6 lines
Quoted 3 lines
No. For example for
exynos5_usbdrd_phy_exit()the driver enables the clock only for the access to PHY block (PHY registers).But I dont get it why we disable code phy clk, which could be used in future phy tune or link control as it part of
CLK_FSYS.
Clock is being disabled when it is not used. AFAIU, current code follows this logic.
Quoted 27 lines
Quoted 18 lines
Quoted 18 lines
[0] https://github.com/torvalds/linux/blob/master/drivers/phy/samsung/phy-exynos5-usbdrd.c#L362-L412
[1] https://github.com/torvalds/linux/blob/master/drivers/phy/samsung/phy-exynos5-usbdrd.c#L424-L446On 3.10.x kernel
clk_summaryall the clk are enables.mout_usbdrd300 3 3 24000000 dout_usbdrd300 2 2 24000000 sclk_usbdrd300 1 1 24000000 dout_usbphy300 2 2 24000000 sclk_usbphy300 1 1 24000000 mout_usbdrd301 3 3 24000000 dout_usbdrd301 2 2 24000000 sclk_usbdrd301 1 1 24000000 dout_usbphy301 2 2 24000000 sclk_usbphy301 1 1 24000000Quoted 7 lines
Quoted 1 line
Some more changes in clk driver might be required to stabilize the usb module.
Please describe the issues with stability first.
Best regards,
KrzysztofOn stability:
Quoted 8 lines
USB 3.0 read / write performance is not up to mark with moderate data read / write speed,
If you give long compilation of kernel or any source code it would likely be slow
It work pretty good with USB 3.0 SSD but generally failed with 2.5' inc HDD.If needed I will share you some performance test result with and without these patches.
Yes, such results are essential for this (and any) commit.
Best regards,
Krzysztof
Quoted 5 lines
I have some more changes related to usbdrd phy change queued along this on.
Best Regards
-Anand
Krzysztof Kozlowski
maintainerQuoted 1 line · Anand Moon
update the usbdrd link control and phy contol clks.
The commit title and especially commit message should explain why you are doing this and what are you doing. "Update" is not enough. Everything could be called update.
Therefore I do not understand the reason behind the patch.
BR,
Krzysztof
Hi Krzysztof,
Quoted 11 lines · Krzysztof Kozlowski
Quoted 1 line · Anand Moon
update the usbdrd link control and phy contol clks.
The commit title and especially commit message should explain why you are doing this and what are you doing. "Update" is not enough. Everything could be called update.
Therefore I do not understand the reason behind the patch.
BR,
Krzysztof
so as per the driver.
@clk: phy clock for register access
@ref_clk: reference clock to PHY block from which PHY's operational clocks are derived
Both CLK_SCLK_USBPHY300 and CLK_SCLK_USBD300 belong to FSYS Clock and CLK_USBD300 clk is being used by the usbdrd dwc3 module.
[0] https://github.com/torvalds/linux/blob/master/drivers/clk/samsung/clk-exynos5420.c#L1040-L1053
So their is mismatch of the clk used by the usbdrd driver. with the above changes the driver work well with camera and disk drives connected to usb 3.0 ports and their is improvement in the performance.
root at odroid:~# lsusb -t
/: Bus 06.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 5000M
/: Bus 05.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 480M
|__ Port 1: Dev 3, If 0, Class=Vendor Specific Class, Driver=r8152, 480M
/: Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 5000M
|__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/2p, 5000M
|__ Port 1: Dev 3, If 0, Class=Mass Storage, Driver=uas, 5000M
|__ Port 2: Dev 4, If 0, Class=Mass Storage, Driver=uas, 5000M
/: Bus 03.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 480M
|__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/2p, 480M
/: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=exynos-ohci/3p, 12M
/: Bus 01.Port 1: Dev 1, Class=root_hub, Driver=exynos-ehci/3p, 480MBest Regards
-Anand
Krzysztof Kozlowski
maintainerQuoted 20 lines · Anand Moon
Hi Krzysztof,
Quoted 11 lines · Krzysztof Kozlowski
Quoted 1 line · Anand Moon
update the usbdrd link control and phy contol clks.
The commit title and especially commit message should explain why you are doing this and what are you doing. "Update" is not enough. Everything could be called update.
Therefore I do not understand the reason behind the patch.
BR,
Krzysztofso as per the driver.
@clk: phy clock for register access
@ref_clk: reference clock to PHY block from which PHY's operational clocks are derived
Quoted 6 lines
Both
CLK_SCLK_USBPHY300andCLK_SCLK_USBD300belong to FSYS Clock andCLK_USBD300clk is being used by the usbdrd dwc3 module.[0] https://github.com/torvalds/linux/blob/master/drivers/clk/samsung/clk-exynos5420.c#L1040-L1053
So their is mismatch of the clk used by the usbdrd driver.
Where is the mismatch? I do not understand.
Quoted 2 lines
with the above changes the driver work well with camera and disk drives connected to usb 3.0 ports and their is improvement in the performance.
If something is not working now, please describe it exactly so we could both reproduce and then observe the end results of fix.
I do not understand how the change of these clocks brings improvement in performance... ok, sometimes it might happen if the rate of clock is being used on bus. Is this the case?
Best regards,
Krzysztof
Quoted 16 lines
root at odroid:~# lsusb -t
/: Bus 06.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 5000M /: Bus 05.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 480M |__ Port 1: Dev 3, If 0, Class=Vendor Specific Class, Driver=r8152, 480M /: Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 5000M |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/2p, 5000M |__ Port 1: Dev 3, If 0, Class=Mass Storage, Driver=uas, 5000M |__ Port 2: Dev 4, If 0, Class=Mass Storage, Driver=uas, 5000M /: Bus 03.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 480M |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/2p, 480M /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=exynos-ohci/3p, 12M /: Bus 01.Port 1: Dev 1, Class=root_hub, Driver=exynos-ehci/3p, 480MBest Regards
-Anand
Hi Krzysztof,
Quoted 36 lines · Krzysztof Kozlowski
Quoted 20 lines · Anand Moon
Hi Krzysztof,
Quoted 11 lines · Krzysztof Kozlowski
Quoted 1 line · Anand Moon
update the usbdrd link control and phy contol clks.
The commit title and especially commit message should explain why you are doing this and what are you doing. "Update" is not enough. Everything could be called update.
Therefore I do not understand the reason behind the patch.
BR,
Krzysztofso as per the driver.
@clk: phy clock for register access
@ref_clk: reference clock to PHY block from which PHY's operational clocks are derivedQuoted 6 lines
Both
CLK_SCLK_USBPHY300andCLK_SCLK_USBD300belong to FSYS Clock andCLK_USBD300clk is being used by the usbdrd dwc3 module.[0] https://github.com/torvalds/linux/blob/master/drivers/clk/samsung/clk-exynos5420.c#L1040-L1053
So their is mismatch of the clk used by the usbdrd driver.
Where is the mismatch? I do not understand.
Quoted 2 lines
with the above changes the driver work well with camera and disk drives connected to usb 3.0 ports and their is improvement in the performance.
If something is not working now, please describe it exactly so we could both reproduce and then observe the end results of fix.
Quoted 7 lines
I do not understand how the change of these clocks brings improvement in performance... ok, sometimes it might happen if the rate of clock is being used on bus. Is this the case?
Best regards,
Krzysztof
[snip]
As per Exynos5422 CMU
CLK_SCLK_USBPHY300 and CLK_SCLK_USBD300 are special clk meant to control the usbdrd
CLK_SCLK_USBPHY300/1 is used for phy control
CLK_SCLK_USBD300/1 is used for link control.
Following link share the details of the CMU_TOP
[0] https://imgur.com/9OKaXEM
On Odroid cloudshell module with old hard disk it time to time do reset of hub. on heavy traffic of data.
[46953.765974] usb 4-1.1: reset SuperSpeed USB device number 3 using xhci-hcd [47006.851310] usb 4-1.1: reset SuperSpeed USB device number 3 using xhci-hcd
[ 730.078441] sd 1:0:0:0: [sdb] tag#5 CDB: opcode=0x2a 2a 00 26 b6 58 80 00 00 80 00 [ 730.078471] sd 1:0:0:0: [sdb] tag#6 uas_zap_pending 0 uas-tag 7 inflight: CMD [ 730.078498] sd 1:0:0:0: [sdb] tag#6 CDB: opcode=0x28 28 00 00 00 20 00 00 04 00 00 [ 730.161071] usb 4-1.2: reset SuperSpeed USB device number 4 using xhci-hcd [ 730.187194] scsi host1: uas_eh_bus_reset_handler success
At some time it work stable but eventfully their is loss of data.
On 3.10.x
It clk gate is tuned to support CLK_GATE_MULTI_BIT_SET, I am not sure how this will apply on 4.x kernel
Best Regards
-Anand
Krzysztof Kozlowski
maintainerQuoted 50 lines · Anand Moon
Hi Krzysztof,
Quoted 36 lines · Krzysztof Kozlowski
Quoted 20 lines · Anand Moon
Hi Krzysztof,
Quoted 11 lines · Krzysztof Kozlowski
Quoted 1 line · Anand Moon
update the usbdrd link control and phy contol clks.
The commit title and especially commit message should explain why you are doing this and what are you doing. "Update" is not enough. Everything could be called update.
Therefore I do not understand the reason behind the patch.
BR,
Krzysztofso as per the driver.
@clk: phy clock for register access
@ref_clk: reference clock to PHY block from which PHY's operational clocks are derivedQuoted 6 lines
Both
CLK_SCLK_USBPHY300andCLK_SCLK_USBD300belong to FSYS Clock andCLK_USBD300clk is being used by the usbdrd dwc3 module.[0] https://github.com/torvalds/linux/blob/master/drivers/clk/samsung/clk-exynos5420.c#L1040-L1053
So their is mismatch of the clk used by the usbdrd driver.
Where is the mismatch? I do not understand.
Quoted 2 lines
with the above changes the driver work well with camera and disk drives connected to usb 3.0 ports and their is improvement in the performance.
If something is not working now, please describe it exactly so we could both reproduce and then observe the end results of fix.
Quoted 7 lines
I do not understand how the change of these clocks brings improvement in performance... ok, sometimes it might happen if the rate of clock is being used on bus. Is this the case?
Best regards,
Krzysztof[snip]
As per Exynos5422 CMU
Quoted 8 lines
CLK_SCLK_USBPHY300andCLK_SCLK_USBD300are special clk meant to control the usbdrdCLK_SCLK_USBPHY300/1 is used for phy control
CLK_SCLK_USBD300/1 is used for link control.Following link share the details of the
CMU_TOP
[0] https://imgur.com/9OKaXEM
... and? Unfortunately this does not explain the problem to me.
Quoted 15 lines
On Odroid cloudshell module with old hard disk it time to time do reset of hub. on heavy traffic of data.
[46953.765974] usb 4-1.1: reset SuperSpeed USB device number 3 using xhci-hcd [47006.851310] usb 4-1.1: reset SuperSpeed USB device number 3 using xhci-hcd[ 730.078441] sd 1:0:0:0: [sdb] tag#5 CDB: opcode=0x2a 2a 00 26 b6 58 80 00 00 80 00 [ 730.078471] sd 1:0:0:0: [sdb] tag#6 uas_zap_pending 0 uas-tag 7 inflight: CMD [ 730.078498] sd 1:0:0:0: [sdb] tag#6 CDB: opcode=0x28 28 00 00 00 20 00 00 04 00 00 [ 730.161071] usb 4-1.2: reset SuperSpeed USB device number 4 using xhci-hcd [ 730.187194] scsi host1: uas_eh_bus_reset_handler success
Quoted 1 line
At some time it work stable but eventfully their is loss of data.
Thanks for the error log. You still did not say it explicitly so I am
guessing: do you mean that these errors are the effect of disabled clock?
Maybe try to explain in simple sentences:
1. What is the visible problem.
2. How to reproduce it (you partially mentioned this here - Odroid cloudshell with some disk)
3. What is the reason behind the problem.
4. How this patch fixes the reason behind (thus fixing the visible problem).
Quoted 6 lines
On 3.10.x
It clk gate is tuned to supportCLK_GATE_MULTI_BIT_SET, I am not sure how this will apply on 4.x kernel
Me neither.
Best regards,
Krzysztof
Quoted 13 lines · Anand Moon
Hi Krzysztof,
Quoted 10 lines · Krzysztof Kozlowski
Quoted 1 line · Anand Moon
update the usbdrd link control and phy contol clks.
The commit title and especially commit message should explain why you are doing this and what are you doing. "Update" is not enough. Everything could be called update.
Therefore I do not understand the reason behind the patch.
BR,
Krzysztof
Quoted 7 lines
so as per the driver.
@clk: phy clock for register access
@ref_clk: reference clock to PHY block from which PHY's operational clocks are derivedBoth
CLK_SCLK_USBPHY300andCLK_SCLK_USBD300belong to FSYS Clock andCLK_USBD300clk is being used by the usbdrd dwc3 module.
From what i vaguely remember, the CLK_SCLK* are the parent clocks going
to the
FSYS block. In this FSYS block the two clocks - CLK_USBD300, and CLK_SCLK_USBPHY300
are coming.
"phy" - represents the AHB clock used only for the register writes, and is required only
during register access. Since we don't need this clock for phy operation, your next change
that removes the clk_disable() sounds incorrect to me.
Just to double check, this AHB clock should be 200MHz (from what i remember) "ref_clk" - the phy reference that clocks the phy PLL. This is a 24MHz clock.
Clubbing the changes in two patches:
- You change the "phy" clock from CLK_USBD300 to CLK_SCLK_USBPHY300, and then
you _had_ to remove the clk_disable().
I think you needed the second patch just because you introduced this
change in the clocks.
- Like Krzysztof mentioned in the thread, if there's a performance improvement you may
want to double check the clock rates.
Best regards
Vivek
Quoted 22 lines
[0] https://github.com/torvalds/linux/blob/master/drivers/clk/samsung/clk-exynos5420.c#L1040-L1053
So their is mismatch of the clk used by the usbdrd driver. with the above changes the driver work well with camera and disk drives connected to usb 3.0 ports and their is improvement in the performance.
root at odroid:~# lsusb -t
/: Bus 06.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 5000M /: Bus 05.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 480M|__ Port 1: Dev 3, If 0, Class=Vendor Specific Class, Driver=r8152, 480M /: Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 5000M |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/2p, 5000M |__ Port 1: Dev 3, If 0, Class=Mass Storage, Driver=uas, 5000M |__ Port 2: Dev 4, If 0, Class=Mass Storage, Driver=uas, 5000M /: Bus 03.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 480M |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/2p, 480M/: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=exynos-ohci/3p, 12M /: Bus 01.Port 1: Dev 1, Class=root_hub, Driver=exynos-ehci/3p, 480M
Best Regards
-Anand
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Hi Vivek,
Quoted 52 lines · Vivek Gautam
Quoted 18 lines · Anand Moon
Hi Krzysztof,
Quoted 5 lines · Krzysztof Kozlowski
Quoted 2 lines · Anand Moon
update the usbdrd link control and phy contol clks.
Quoted 8 lines
The commit title and especially commit message should explain why you are doing this and what are you doing. "Update" is not enough. Everything could be called update.
Therefore I do not understand the reason behind the patch.
BR,
KrzysztofQuoted 7 lines
so as per the driver.
@clk: phy clock for register access
@ref_clk: reference clock to PHY block from which PHY's operational clocks are derivedBoth
CLK_SCLK_USBPHY300andCLK_SCLK_USBD300belong to FSYS Clock andCLK_USBD300clk is being used by the usbdrd dwc3 module.From what i vaguely remember, the CLK_SCLK* are the parent clocks going to the
FSYS block. In this FSYS block the two clocks -CLK_USBD300, andCLK_SCLK_USBPHY300
are coming."phy" - represents the AHB clock used only for the register writes, and is required only
during register access. Since we don't need this clock for phy operation, your next change
that removes theclk_disable()sounds incorrect to me.
Just to double check, this AHB clock should be 200MHz (from what i remember) "ref_clk" - the phy reference that clocks the phy PLL. This is a 24MHz clock.Clubbing the changes in two patches:
- You change the "phy" clock fromCLK_USBD300toCLK_SCLK_USBPHY300, and then
you _had_ to remove theclk_disable().
I think you needed the second patch just because you introduced this
change in the clocks.
Quoted 4 lines
- Like Krzysztof mentioned in the thread, if there's a performance improvement you may
want to double check the clock rates.
Yes their is slight improvement with these changes
I will share my test result once I add few more changes to this drive.
Quoted 4 lines
Best regards
Vivek
Thank for your explanation on the clk internals. I have read few detail on the initial mainline list.
[0] https://lkml.org/lkml/2014/4/8/247
CLK_GATE_TOP_SCLK_FSYS
SCLK_USBDRD301 Gating SUSPEND_CLK for USBDRD30_1SCLK_USBDRD300 Gating SUSPEND_CLK for USBDRD30_0SCLK_USBPHY300 Gating USB30_SCLK_100M for USBDRD30_PHY_0
Gating USB20_PICO_CLKCORE
for PICO PHYSCLK_USBPHY300 Gating USB30_SCLK_100M for USBDRD30_PHY_1
So we did not considered the SUSPEN_CLK for phy.
Below is the clk structure diagram for usb drd phy.
[1] https://lkml.org/lkml/2014/4/10/240
Here is how it shown in manual.
___________________
| |
SUSPEND_CLK | ____________ |
------------------------- | | PHY | |
| | controller
|------|--------------------------------------------------------
| |___________| |
|
|
|
|
|
|
|
| USB 3.0
| USB30_SCLK_100M------------- |-------------------------------|
| DRD
| |
|---->vbus
---------------------------------
|
|
| Controller
| |
|---------------------| |
|
| Pipe interface | |
USB 3.0 DRD | |
| ----------------
|-----------------------------------------------|
|____________| |
| | PHY |
| UTMI+ Interface |
|
| | Link cont. |
|-----------------------------------------------|
|
| |-----------------|
|
|__________________|
|
|
|__________________|So how can we support SUSPEND_CLK ?
Do we need to keep this SUSPEND_CLK enable ?
As of now my dts change are wrong
How about below changes.
&usbdrd_phy0 {
- clocks = <&clock CLK_USBD300>, <&clock CLK_SCLK_USBPHY300>;
+ clocks = <&clock CLK_SCLK_USBD300>, <&clock CLK_SCLK_USBPHY300>;
clock-names = "phy", "ref";
samsung,pmu-syscon = <&pmu_system_controller>; };
@@ -1476,7 +1476,7 @@
};
&usbdrd_phy1 {
- clocks = <&clock CLK_USBD301>, <&clock CLK_SCLK_USBPHY301>;
+ clocks = <&clock CLK_SCLK_USBD301>, <&clock CLK_SCLK_USBPHY301>;
clock-names = "phy", "ref";
samsung,pmu-syscon = <&pmu_system_controller>;};
Best Regards
-Anand