[RFC 1/2] ARM: dts: exynos: update the usbdrd phy and ref clk

[RFC 1/2] ARM: dts: exynos: update the usbdrd phy and ref clk

[Anand Moon]

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(-)

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>;
 };
-- 
2.7.4

[RFC 2/2] phy: exynos5-usbdrd: remove disable and enable of phy clk

[Anand Moon]

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

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 drivers/phy/samsung/phy-exynos5-usbdrd.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/phy/samsung/phy-exynos5-usbdrd.c b/drivers/phy/samsung/phy-exynos5-usbdrd.c
index 22c68f5..5e8054c 100644
--- a/drivers/phy/samsung/phy-exynos5-usbdrd.c
+++ b/drivers/phy/samsung/phy-exynos5-usbdrd.c
@@ -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

[Krzysztof Kozlowski]

On Fri, Oct 6, 2017 at 6:36 AM, Anand Moon <linux.amoon-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 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

>
> 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
>
> Signed-off-by: Anand Moon <linux.amoon-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/phy/samsung/phy-exynos5-usbdrd.c | 7 -------
>  1 file changed, 7 deletions(-)
>
> diff --git a/drivers/phy/samsung/phy-exynos5-usbdrd.c b/drivers/phy/samsung/phy-exynos5-usbdrd.c
> index 22c68f5..5e8054c 100644
> --- a/drivers/phy/samsung/phy-exynos5-usbdrd.c
> +++ b/drivers/phy/samsung/phy-exynos5-usbdrd.c
> @@ -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
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 2/2] phy: exynos5-usbdrd: remove disable and enable of phy clk

[Anand Moon]

Hi Krzysztof,

On 6 October 2017 at 12:12, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Fri, Oct 6, 2017 at 6:36 AM, Anand Moon <linux.amoon@gmail.com> wrote:
>> 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
>

[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            24000000

Some more changes in clk driver might be required to stabilize the usb module.

Best Regards
-Anand

Re: [RFC 2/2] phy: exynos5-usbdrd: remove disable and enable of phy clk

[Krzysztof Kozlowski]

On Sun, Oct 08, 2017 at 06:11:12PM +0530, Anand Moon wrote:
> Hi Krzysztof,
> 
> On 6 October 2017 at 12:12, Krzysztof Kozlowski <krzk-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Fri, Oct 6, 2017 at 6:36 AM, Anand Moon <linux.amoon-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> 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
> >
> 
> [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).

> 
> [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            24000000
> 
> Some more changes in clk driver might be required to stabilize the usb module.

Please describe the issues with stability first.

Best regards,
Krzysztof

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 2/2] phy: exynos5-usbdrd: remove disable and enable of phy clk

[Anand Moon]

Hi Krzysztof,

On 8 October 2017 at 21:20, Krzysztof Kozlowski <krzk-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Sun, Oct 08, 2017 at 06:11:12PM +0530, Anand Moon wrote:
>> Hi Krzysztof,
>>
>> On 6 October 2017 at 12:12, Krzysztof Kozlowski <krzk-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> > On Fri, Oct 6, 2017 at 6:36 AM, Anand Moon <linux.amoon-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >> 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
>> >
>>
>> [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.

>>
>> [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            24000000
>>
>> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 2/2] phy: exynos5-usbdrd: remove disable and enable of phy clk

[Anand Moon]

hi Krzysztof,

On 9 October 2017 at 02:37, Anand Moon <linux.amoon@gmail.com> wrote:
> Hi Krzysztof,
>
> On 8 October 2017 at 21:20, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> On Sun, Oct 08, 2017 at 06:11:12PM +0530, Anand Moon wrote:
>>> Hi Krzysztof,
>>>
>>> On 6 October 2017 at 12:12, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>> > On Fri, Oct 6, 2017 at 6:36 AM, Anand Moon <linux.amoon@gmail.com> wrote:
>>> >> 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
>>> >
>>>
>>> [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.

>> 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.
>
>>>
>>> [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            24000000
>>>
>>> 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

[Krzysztof Kozlowski]

On Mon, Oct 09, 2017 at 08:46:48PM +0530, Anand Moon wrote:
> hi Krzysztof,
> 
> On 9 October 2017 at 02:37, Anand Moon <linux.amoon@gmail.com> wrote:
> > Hi Krzysztof,
> >
> > On 8 October 2017 at 21:20, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >> On Sun, Oct 08, 2017 at 06:11:12PM +0530, Anand Moon wrote:
> >>> Hi Krzysztof,
> >>>
> >>> On 6 October 2017 at 12:12, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>> > On Fri, Oct 6, 2017 at 6:36 AM, Anand Moon <linux.amoon@gmail.com> wrote:
> >>> >> 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
> >>> >
> >>>
> >>> [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

Re: [RFC 2/2] phy: exynos5-usbdrd: remove disable and enable of phy clk

[Krzysztof Kozlowski]

On Mon, Oct 09, 2017 at 02:37:14AM +0530, Anand Moon wrote:
> Hi Krzysztof,
> 
> On 8 October 2017 at 21:20, Krzysztof Kozlowski <krzk-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Sun, Oct 08, 2017 at 06:11:12PM +0530, Anand Moon wrote:
> >> Hi Krzysztof,
> >>
> >> On 6 October 2017 at 12:12, Krzysztof Kozlowski <krzk-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >> > On Fri, Oct 6, 2017 at 6:36 AM, Anand Moon <linux.amoon-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> >> 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
> >> >
> >>
> >> [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.

Clock is being disabled when it is not used. AFAIU, current code follows
this logic.

> 
> >>
> >> [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            24000000
> >>
> >> 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.

Yes, such results are essential for this (and any) commit.

Best regards,
Krzysztof


> 
> I have some more changes related to usbdrd phy change queued along this on.
> 
> Best Regards
> -Anand
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/2] ARM: dts: exynos: update the usbdrd phy and ref clk

[Krzysztof Kozlowski]

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

Re: [RFC 1/2] ARM: dts: exynos: update the usbdrd phy and ref clk

[Anand Moon]

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

Re: [RFC 1/2] ARM: dts: exynos: update the usbdrd phy and ref clk

[Krzysztof Kozlowski]

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

Re: [RFC 1/2] ARM: dts: exynos: update the usbdrd phy and ref clk

[Anand Moon]

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

Re: [RFC 1/2] ARM: dts: exynos: update the usbdrd phy and ref clk

[Krzysztof Kozlowski]

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

Re: [RFC 1/2] ARM: dts: exynos: update the usbdrd phy and ref clk

[Vivek Gautam]

On 10/08/2017 06:06 PM, Anand Moon wrote:
> Hi Krzysztof,
>
> On 6 October 2017 at 12:08, Krzysztof Kozlowski <krzk-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> On Fri, Oct 6, 2017 at 6:36 AM, Anand Moon <linux.amoon-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/2] ARM: dts: exynos: update the usbdrd phy and ref clk

[Anand Moon]

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