| Message ID | 1507264595-3565-1-git-send-email-linux.amoon@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Fri, Oct 6, 2017 at 6:36 AM, Anand Moon <linux.amoon@gmail.com> wrote:
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Krzysztof, On 6 October 2017 at 12:08, Krzysztof Kozlowski <krzk@kernel.org> wrote: > On Fri, Oct 6, 2017 at 6:36 AM, Anand Moon <linux.amoon@gmail.com> wrote: >> 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@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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Oct 08, 2017 at 06:06:19PM +0530, Anand Moon wrote: > Hi Krzysztof, > > On 6 October 2017 at 12:08, Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On Fri, Oct 6, 2017 at 6:36 AM, Anand Moon <linux.amoon@gmail.com> wrote: > >> 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. Where is the mismatch? I do not understand. > 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 > > root@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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Krzysztof, On 8 October 2017 at 21:17, Krzysztof Kozlowski <krzk@kernel.org> wrote: > On Sun, Oct 08, 2017 at 06:06:19PM +0530, Anand Moon wrote: >> Hi Krzysztof, >> >> On 6 October 2017 at 12:08, Krzysztof Kozlowski <krzk@kernel.org> wrote: >> > On Fri, Oct 6, 2017 at 6:36 AM, Anand Moon <linux.amoon@gmail.com> wrote: >> >> 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. > > Where is the mismatch? I do not understand. > >> 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 > [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 [1] https://github.com/hardkernel/linux/blob/odroidxu3-3.10.y/drivers/clk/samsung/clk-exynos5422.c#L1771-L1777 Best Regards -Anand -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 09, 2017 at 02:36:13AM +0530, Anand Moon wrote: > Hi Krzysztof, > > On 8 October 2017 at 21:17, Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On Sun, Oct 08, 2017 at 06:06:19PM +0530, Anand Moon wrote: > >> Hi Krzysztof, > >> > >> On 6 October 2017 at 12:08, Krzysztof Kozlowski <krzk@kernel.org> wrote: > >> > On Fri, Oct 6, 2017 at 6:36 AM, Anand Moon <linux.amoon@gmail.com> wrote: > >> >> 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. > > > > Where is the mismatch? I do not understand. > > > >> 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 > > > [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 ... and? Unfortunately this does not explain the problem to me. > > 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. 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). > > 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 > > [1] https://github.com/hardkernel/linux/blob/odroidxu3-3.10.y/drivers/clk/samsung/clk-exynos5422.c#L1771-L1777 Me neither. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/08/2017 06:06 PM, Anand Moon wrote: > Hi Krzysztof, > > On 6 October 2017 at 12:08, Krzysztof Kozlowski <krzk@kernel.org> wrote: >> On Fri, Oct 6, 2017 at 6:36 AM, Anand Moon <linux.amoon@gmail.com> wrote: >>> 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. 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 > > [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@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
Hi Vivek, On 10 October 2017 at 11:57, Vivek Gautam <vivek.gautam@codeaurora.org> wrote: > > > On 10/08/2017 06:06 PM, Anand Moon wrote: >> >> Hi Krzysztof, >> >> On 6 October 2017 at 12:08, Krzysztof Kozlowski <krzk@kernel.org> wrote: >>> >>> On Fri, Oct 6, 2017 at 6:36 AM, Anand Moon <linux.amoon@gmail.com> wrote: >>>> >>>> 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. > > > 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. > Yes their is slight improvement with these changes I will share my test result once I add few more changes to this drive. > > 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_1 SCLK_USBDRD300 Gating SUSPEND_CLK for USBDRD30_0 SCLK_USBPHY300 Gating USB30_SCLK_100M for USBDRD30_PHY_0 Gating USB20_PICO_CLKCORE for PICO PHY SCLK_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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/boot/dts/exynos5410.dtsi b/arch/arm/boot/dts/exynos5410.dtsi index 7eab4bc..5af9f4b 100644 --- a/arch/arm/boot/dts/exynos5410.dtsi +++ b/arch/arm/boot/dts/exynos5410.dtsi @@ -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>; }; diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi index 88e5d6d..36d26ab 100644 --- a/arch/arm/boot/dts/exynos5420.dtsi +++ b/arch/arm/boot/dts/exynos5420.dtsi @@ -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>; };
update the usbdrd link control and phy contol clks. Signed-off-by: Anand Moon <linux.amoon@gmail.com> --- 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(-)