mbox series

[PATCHv3,0/3] Add support for suspend clk for Exynos5422 SoC

Message ID 20200210105108.1128-1-linux.amoon@gmail.com
Headers show
Series Add support for suspend clk for Exynos5422 SoC | expand

Message

Anand Moon Feb. 10, 2020, 10:51 a.m. UTC
Long time ago I tried to add suspend clk for dwc3 phy
which was wrong appoch, see below.

[0] https://lore.kernel.org/patchwork/patch/837635/
[1] https://lore.kernel.org/patchwork/patch/837636/

This patch series tries to enable suspend clk using 
exynos dwc3 driver, for this I have added new 
compatible string "samsung,exynos5420-dwusb3"
so that we could add new suspend clk in addition
to the core clk. exynos dwc3 driver will help
enable/disable these clk.

-Anand

Anand Moon (3):
  devicetree: bindings: exynos: Add new compatible for Exynos5420 dwc3
    clocks support
  ARM: dts: exynos: Add missing usbdrd3 suspend clk
  usb: dwc3: exynos: Add support for Exynos5422 suspend clk

 Documentation/devicetree/bindings/usb/exynos-usb.txt | 4 +++-
 arch/arm/boot/dts/exynos5420.dtsi                    | 8 ++++----
 arch/arm/boot/dts/exynos54xx.dtsi                    | 4 ++--
 drivers/usb/dwc3/dwc3-exynos.c                       | 9 +++++++++
 4 files changed, 18 insertions(+), 7 deletions(-)

Comments

Anand Moon Feb. 10, 2020, 10:56 a.m. UTC | #1
Hi All,

Sorry typo this patch series should be PATCHv1 and not PATCHv3

-Anand

On Mon, 10 Feb 2020 at 16:21, Anand Moon <linux.amoon@gmail.com> wrote:
>
> Long time ago I tried to add suspend clk for dwc3 phy
> which was wrong appoch, see below.
>
> [0] https://lore.kernel.org/patchwork/patch/837635/
> [1] https://lore.kernel.org/patchwork/patch/837636/
>
> This patch series tries to enable suspend clk using
> exynos dwc3 driver, for this I have added new
> compatible string "samsung,exynos5420-dwusb3"
> so that we could add new suspend clk in addition
> to the core clk. exynos dwc3 driver will help
> enable/disable these clk.
>
> -Anand
>
> Anand Moon (3):
>   devicetree: bindings: exynos: Add new compatible for Exynos5420 dwc3
>     clocks support
>   ARM: dts: exynos: Add missing usbdrd3 suspend clk
>   usb: dwc3: exynos: Add support for Exynos5422 suspend clk
>
>  Documentation/devicetree/bindings/usb/exynos-usb.txt | 4 +++-
>  arch/arm/boot/dts/exynos5420.dtsi                    | 8 ++++----
>  arch/arm/boot/dts/exynos54xx.dtsi                    | 4 ++--
>  drivers/usb/dwc3/dwc3-exynos.c                       | 9 +++++++++
>  4 files changed, 18 insertions(+), 7 deletions(-)
>
> --
> 2.25.0
>
Krzysztof Kozlowski Feb. 10, 2020, 1:50 p.m. UTC | #2
On Mon, Feb 10, 2020 at 10:51:07AM +0000, Anand Moon wrote:
> This patch adds new combatible strings for USBDRD3
> for adding missing suspend clk, exynos5422 usbdrd3
> support two clk USBD300 and SCLK_USBD300, so add missing
> suspemd_clk for Exynos542x DWC3 nodes.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>  arch/arm/boot/dts/exynos5420.dtsi | 8 ++++----
>  arch/arm/boot/dts/exynos54xx.dtsi | 4 ++--
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
> index b672080e7469..bd505256a223 100644
> --- a/arch/arm/boot/dts/exynos5420.dtsi
> +++ b/arch/arm/boot/dts/exynos5420.dtsi
> @@ -1372,8 +1372,8 @@ &trng {
>  };
>  
>  &usbdrd3_0 {
> -	clocks = <&clock CLK_USBD300>;
> -	clock-names = "usbdrd30";
> +	clocks = <&clock CLK_USBD300>, <&clock CLK_SCLK_USBD300>;
> +	clock-names = "usbdrd30", "usbdrd30_susp_clk";
>  };
>  
>  &usbdrd_phy0 {
> @@ -1383,8 +1383,8 @@ &usbdrd_phy0 {
>  };
>  
>  &usbdrd3_1 {
> -	clocks = <&clock CLK_USBD301>;
> -	clock-names = "usbdrd30";
> +	clocks = <&clock CLK_USBD301>, <&clock CLK_SCLK_USBD301>;
> +	clock-names = "usbdrd30", "usbdrd30_susp_clk";
>  };
>  
>  &usbdrd_dwc3_1 {
> diff --git a/arch/arm/boot/dts/exynos54xx.dtsi b/arch/arm/boot/dts/exynos54xx.dtsi
> index 8aa5117e58ce..0aac6255de5d 100644
> --- a/arch/arm/boot/dts/exynos54xx.dtsi
> +++ b/arch/arm/boot/dts/exynos54xx.dtsi
> @@ -143,7 +143,7 @@ hsi2c_7: i2c@12cd0000 {
>  		};
>  
>  		usbdrd3_0: usb3-0 {
> -			compatible = "samsung,exynos5250-dwusb3";
> +			compatible = "samsung,exynos5420-dwusb3";
>  			#address-cells = <1>;
>  			#size-cells = <1>;
>  			ranges;
> @@ -165,7 +165,7 @@ usbdrd_phy0: phy@12100000 {
>  		};
>  
>  		usbdrd3_1: usb3-1 {
> -			compatible = "samsung,exynos5250-dwusb3";
> +			compatible = "samsung,exynos5420-dwusb3";

This affects also Exynos5410 but you do not add new clock there.

Best regards,
Krzysztof
Krzysztof Kozlowski Feb. 10, 2020, 1:56 p.m. UTC | #3
On Mon, Feb 10, 2020 at 10:51:05AM +0000, Anand Moon wrote:
> Long time ago I tried to add suspend clk for dwc3 phy
> which was wrong appoch, see below.
> 
> [0] https://lore.kernel.org/patchwork/patch/837635/
> [1] https://lore.kernel.org/patchwork/patch/837636/
> 

You ignored parts of my review from these previous patches. I asked for
describing WHY are you doing this and WHAT problem are you trying to
solve. I asked for this multiple times. Unfortunately I cannot find the
answers to my questions in this patchset...

Best regards,
Krzysztof


> This patch series tries to enable suspend clk using 
> exynos dwc3 driver, for this I have added new 
> compatible string "samsung,exynos5420-dwusb3"
> so that we could add new suspend clk in addition
> to the core clk. exynos dwc3 driver will help
> enable/disable these clk.
> 
> -Anand
> 
> Anand Moon (3):
>   devicetree: bindings: exynos: Add new compatible for Exynos5420 dwc3
>     clocks support
>   ARM: dts: exynos: Add missing usbdrd3 suspend clk
>   usb: dwc3: exynos: Add support for Exynos5422 suspend clk
> 
>  Documentation/devicetree/bindings/usb/exynos-usb.txt | 4 +++-
>  arch/arm/boot/dts/exynos5420.dtsi                    | 8 ++++----
>  arch/arm/boot/dts/exynos54xx.dtsi                    | 4 ++--
>  drivers/usb/dwc3/dwc3-exynos.c                       | 9 +++++++++
>  4 files changed, 18 insertions(+), 7 deletions(-)
> 
> -- 
> 2.25.0
>
Anand Moon Feb. 10, 2020, 5:08 p.m. UTC | #4
Hi Krzysztof,

On Mon, 10 Feb 2020 at 19:26, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Mon, Feb 10, 2020 at 10:51:05AM +0000, Anand Moon wrote:
> > Long time ago I tried to add suspend clk for dwc3 phy
> > which was wrong appoch, see below.
> >
> > [0] https://lore.kernel.org/patchwork/patch/837635/
> > [1] https://lore.kernel.org/patchwork/patch/837636/
> >
>

Thanks for your review comments.

> You ignored parts of my review from these previous patches. I asked for
> describing WHY are you doing this and WHAT problem are you trying to
> solve. I asked for this multiple times. Unfortunately I cannot find the
> answers to my questions in this patchset...
>
> Best regards,
> Krzysztof

I dont know how to resolve this issue, but I want to re-post
some of my changes back for review. let me try again.

My future goal is to add #power-domain for FSYS and FSYS2
which I am trying to resolve some issue.
Also add run-time power management for USB3 drivers.

Here is the clk diagram for FSYS clk as per Exynos5422 user manual.
[0] https://imgur.com/gallery/zAiBoyh

As per the USB 3.0 Architecture T I.

2.13.1 PHY Power Management
The SS PHY has power states P0, P1, P2, and P3, corresponding to the
SS LPM states of U0, U1, U2,and U3. In the P3 state,SS PHY does not drive
the default functional clock,instead, the *susp_clk* is used in its place.

So enable the suspend clk help control the power management
states for the DWC3 controller.

-Anand
Anand Moon Feb. 10, 2020, 5:13 p.m. UTC | #5
Hi Krzysztof,

Thanks for your review comments.

On Mon, 10 Feb 2020 at 19:20, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Mon, Feb 10, 2020 at 10:51:07AM +0000, Anand Moon wrote:
> > This patch adds new combatible strings for USBDRD3
> > for adding missing suspend clk, exynos5422 usbdrd3
> > support two clk USBD300 and SCLK_USBD300, so add missing
> > suspemd_clk for Exynos542x DWC3 nodes.
> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> >  arch/arm/boot/dts/exynos5420.dtsi | 8 ++++----
> >  arch/arm/boot/dts/exynos54xx.dtsi | 4 ++--
> >  2 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
> > index b672080e7469..bd505256a223 100644
> > --- a/arch/arm/boot/dts/exynos5420.dtsi
> > +++ b/arch/arm/boot/dts/exynos5420.dtsi
> > @@ -1372,8 +1372,8 @@ &trng {
> >  };
> >
> >  &usbdrd3_0 {
> > -     clocks = <&clock CLK_USBD300>;
> > -     clock-names = "usbdrd30";
> > +     clocks = <&clock CLK_USBD300>, <&clock CLK_SCLK_USBD300>;
> > +     clock-names = "usbdrd30", "usbdrd30_susp_clk";
> >  };
> >
> >  &usbdrd_phy0 {
> > @@ -1383,8 +1383,8 @@ &usbdrd_phy0 {
> >  };
> >
> >  &usbdrd3_1 {
> > -     clocks = <&clock CLK_USBD301>;
> > -     clock-names = "usbdrd30";
> > +     clocks = <&clock CLK_USBD301>, <&clock CLK_SCLK_USBD301>;
> > +     clock-names = "usbdrd30", "usbdrd30_susp_clk";
> >  };
> >
> >  &usbdrd_dwc3_1 {
> > diff --git a/arch/arm/boot/dts/exynos54xx.dtsi b/arch/arm/boot/dts/exynos54xx.dtsi
> > index 8aa5117e58ce..0aac6255de5d 100644
> > --- a/arch/arm/boot/dts/exynos54xx.dtsi
> > +++ b/arch/arm/boot/dts/exynos54xx.dtsi
> > @@ -143,7 +143,7 @@ hsi2c_7: i2c@12cd0000 {
> >               };
> >
> >               usbdrd3_0: usb3-0 {
> > -                     compatible = "samsung,exynos5250-dwusb3";
> > +                     compatible = "samsung,exynos5420-dwusb3";
> >                       #address-cells = <1>;
> >                       #size-cells = <1>;
> >                       ranges;
> > @@ -165,7 +165,7 @@ usbdrd_phy0: phy@12100000 {
> >               };
> >
> >               usbdrd3_1: usb3-1 {
> > -                     compatible = "samsung,exynos5250-dwusb3";
> > +                     compatible = "samsung,exynos5420-dwusb3";
>
> This affects also Exynos5410 but you do not add new clock there.
>
> Best regards,
> Krzysztof
>

Ok I will update this Exynos5410 dts.

Is samsung,exynos54xx-dwusb3 is valid compatible string
for both the SoC.

-Anand
Krzysztof Kozlowski Feb. 10, 2020, 6:54 p.m. UTC | #6
On Mon, Feb 10, 2020 at 10:43:45PM +0530, Anand Moon wrote:
> Hi Krzysztof,
> 
> Thanks for your review comments.
> 
> On Mon, 10 Feb 2020 at 19:20, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > On Mon, Feb 10, 2020 at 10:51:07AM +0000, Anand Moon wrote:
> > > This patch adds new combatible strings for USBDRD3
> > > for adding missing suspend clk, exynos5422 usbdrd3
> > > support two clk USBD300 and SCLK_USBD300, so add missing
> > > suspemd_clk for Exynos542x DWC3 nodes.
> > >
> > > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > > ---
> > >  arch/arm/boot/dts/exynos5420.dtsi | 8 ++++----
> > >  arch/arm/boot/dts/exynos54xx.dtsi | 4 ++--
> > >  2 files changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
> > > index b672080e7469..bd505256a223 100644
> > > --- a/arch/arm/boot/dts/exynos5420.dtsi
> > > +++ b/arch/arm/boot/dts/exynos5420.dtsi
> > > @@ -1372,8 +1372,8 @@ &trng {
> > >  };
> > >
> > >  &usbdrd3_0 {
> > > -     clocks = <&clock CLK_USBD300>;
> > > -     clock-names = "usbdrd30";
> > > +     clocks = <&clock CLK_USBD300>, <&clock CLK_SCLK_USBD300>;
> > > +     clock-names = "usbdrd30", "usbdrd30_susp_clk";
> > >  };
> > >
> > >  &usbdrd_phy0 {
> > > @@ -1383,8 +1383,8 @@ &usbdrd_phy0 {
> > >  };
> > >
> > >  &usbdrd3_1 {
> > > -     clocks = <&clock CLK_USBD301>;
> > > -     clock-names = "usbdrd30";
> > > +     clocks = <&clock CLK_USBD301>, <&clock CLK_SCLK_USBD301>;
> > > +     clock-names = "usbdrd30", "usbdrd30_susp_clk";
> > >  };
> > >
> > >  &usbdrd_dwc3_1 {
> > > diff --git a/arch/arm/boot/dts/exynos54xx.dtsi b/arch/arm/boot/dts/exynos54xx.dtsi
> > > index 8aa5117e58ce..0aac6255de5d 100644
> > > --- a/arch/arm/boot/dts/exynos54xx.dtsi
> > > +++ b/arch/arm/boot/dts/exynos54xx.dtsi
> > > @@ -143,7 +143,7 @@ hsi2c_7: i2c@12cd0000 {
> > >               };
> > >
> > >               usbdrd3_0: usb3-0 {
> > > -                     compatible = "samsung,exynos5250-dwusb3";
> > > +                     compatible = "samsung,exynos5420-dwusb3";
> > >                       #address-cells = <1>;
> > >                       #size-cells = <1>;
> > >                       ranges;
> > > @@ -165,7 +165,7 @@ usbdrd_phy0: phy@12100000 {
> > >               };
> > >
> > >               usbdrd3_1: usb3-1 {
> > > -                     compatible = "samsung,exynos5250-dwusb3";
> > > +                     compatible = "samsung,exynos5420-dwusb3";
> >
> > This affects also Exynos5410 but you do not add new clock there.
> >
> > Best regards,
> > Krzysztof
> >
> 
> Ok I will update this Exynos5410 dts.
> 
> Is samsung,exynos54xx-dwusb3 is valid compatible string
> for both the SoC.

The compatible should not be wildcard so keep it as
samsung,exynos5420-dwusb3.

Best regards,
Krzysztof
Krzysztof Kozlowski Feb. 10, 2020, 7:02 p.m. UTC | #7
On Mon, Feb 10, 2020 at 10:38:52PM +0530, Anand Moon wrote:
> Hi Krzysztof,
> 
> On Mon, 10 Feb 2020 at 19:26, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > On Mon, Feb 10, 2020 at 10:51:05AM +0000, Anand Moon wrote:
> > > Long time ago I tried to add suspend clk for dwc3 phy
> > > which was wrong appoch, see below.
> > >
> > > [0] https://lore.kernel.org/patchwork/patch/837635/
> > > [1] https://lore.kernel.org/patchwork/patch/837636/
> > >
> >
> 
> Thanks for your review comments.
> 
> > You ignored parts of my review from these previous patches. I asked for
> > describing WHY are you doing this and WHAT problem are you trying to
> > solve. I asked for this multiple times. Unfortunately I cannot find the
> > answers to my questions in this patchset...
> >
> > Best regards,
> > Krzysztof
> 
> I dont know how to resolve this issue, but I want to re-post
> some of my changes back for review. let me try again.
> 
> My future goal is to add #power-domain for FSYS and FSYS2
> which I am trying to resolve some issue.
> Also add run-time power management for USB3 drivers.

You can start by describing why FSYS and FSYS2 power domains cannot be
added right now. Maybe this patchset allows this later?

> 
> Here is the clk diagram for FSYS clk as per Exynos5422 user manual.
> [0] https://imgur.com/gallery/zAiBoyh
> 
> As per the USB 3.0 Architecture T I.
> 
> 2.13.1 PHY Power Management
> The SS PHY has power states P0, P1, P2, and P3, corresponding to the
> SS LPM states of U0, U1, U2,and U3. In the P3 state,SS PHY does not drive
> the default functional clock,instead, the *susp_clk* is used in its place.
> 
> So enable the suspend clk help control the power management
> states for the DWC3 controller.

That's too vague because clock usually cannot "help"... The wording is
wrong and the actual problem is not described.

I could guess from your description and driver behavior that SCLK has to
be on during USB DRD suspend.

Best regards,
Krzysztof