Hi Vivek, On 10 October 2017 at 11:57, Vivek Gautam <vivek.gautam@xxxxxxxxxxxxxx> wrote: > > > On 10/08/2017 06:06 PM, Anand Moon wrote: >> >> Hi Krzysztof, >> >> On 6 October 2017 at 12:08, Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote: >>> >>> On Fri, Oct 6, 2017 at 6:36 AM, Anand Moon <linux.amoon@xxxxxxxxx> 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 devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
