| Message ID | 1507264595-3565-2-git-send-email-linux.amoon@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > > 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 >
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
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. 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
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. > > 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
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
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@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. > > > > 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
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
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;
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(-)