Re: [PATCH 1/2] usb: dwc2: rename DWC2_POWER_DOWN_PARAM_NONE state

From: Artur Petrosyan
Date: Thu Aug 05 2021 - 09:49:33 EST

Next message: Chao Yu: "Re: [PATCH] f2fs: Kconfig: clean up config options about compression"
Previous message: chenguanyou: "[PATCH v2] hungtask: add filter kthread"
In reply to: Greg KH: "Re: [PATCH 1/2] usb: dwc2: rename DWC2_POWER_DOWN_PARAM_NONE state"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Marek,

On 8/4/2021 3:44 PM, Marek Szyprowski wrote:
> DWC2_POWER_DOWN_PARAM_NONE really means that the driver still uses clock
> gating to save power when hardware is not used. Rename the state name to
> DWC2_POWER_DOWN_PARAM_CLOCK_GATING to match the driver behavior.
>
> Suggested-by: Minas Harutyunyan <Minas.Harutyunyan@xxxxxxxxxxxx>
> Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> ---
> This is a follow-up of this discussion:
> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/26099de1-826f-42bf-0de7-759a47faf4a0@xxxxxxxxxxx/__;!!A4F2R9G_pg!MIGkDa9hfT_3k8EATqQs_UYCb7yXMN18CC-gsrNOMI8NqZzAok3DZA6G04fgkS_4H52snpA$
>
> This should be applied on top of v5.14-rc3.
> ---
> drivers/usb/dwc2/core.h | 4 ++--
> drivers/usb/dwc2/core_intr.c | 8 ++++----
> drivers/usb/dwc2/hcd.c | 10 +++++-----
> drivers/usb/dwc2/params.c | 22 +++++++++++-----------
> drivers/usb/dwc2/platform.c | 2 +-
> 5 files changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index cb9059a8444b..38b6733d26ec 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -382,7 +382,7 @@ enum dwc2_ep0_state {
> * If power_down is enabled, the controller will enter
> * power_down in both peripheral and host mode when
> * needed.
> - * 0 - No (default)
> + * 0 - Clock gating (default)
> * 1 - Partial power down
> * 2 - Hibernation
> * @no_clock_gating: Specifies whether to avoid clock gating feature.
> @@ -482,7 +482,7 @@ struct dwc2_core_params {
> bool external_id_pin_ctl;
>
> int power_down;
> -#define DWC2_POWER_DOWN_PARAM_NONE 0
> +#define DWC2_POWER_DOWN_PARAM_CLOCK_GATING 0
> #define DWC2_POWER_DOWN_PARAM_PARTIAL 1
> #define DWC2_POWER_DOWN_PARAM_HIBERNATION 2
> bool no_clock_gating;
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index a5c52b237e72..660abff1d42b 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -327,7 +327,7 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
>
> /* Exit gadget mode clock gating. */
> if (hsotg->params.power_down ==
> - DWC2_POWER_DOWN_PARAM_NONE && hsotg->bus_suspended)
> + DWC2_POWER_DOWN_PARAM_CLOCK_GATING && hsotg->bus_suspended)
> dwc2_gadget_exit_clock_gating(hsotg, 0);
> }
>
> @@ -438,7 +438,7 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>
> /* Exit gadget mode clock gating. */
> if (hsotg->params.power_down ==
> - DWC2_POWER_DOWN_PARAM_NONE && hsotg->bus_suspended)
> + DWC2_POWER_DOWN_PARAM_CLOCK_GATING && hsotg->bus_suspended)
> dwc2_gadget_exit_clock_gating(hsotg, 0);
> } else {
> /* Change to L0 state */
> @@ -455,7 +455,7 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
> }
>
> if (hsotg->params.power_down ==
> - DWC2_POWER_DOWN_PARAM_NONE && hsotg->bus_suspended)
> + DWC2_POWER_DOWN_PARAM_CLOCK_GATING && hsotg->bus_suspended)
> dwc2_host_exit_clock_gating(hsotg, 1);
>
> /*
> @@ -551,7 +551,7 @@ static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg)
> dev_err(hsotg->dev,
> "enter hibernation failed\n");
> break;
> - case DWC2_POWER_DOWN_PARAM_NONE:
> + case DWC2_POWER_DOWN_PARAM_CLOCK_GATING:
> /*
> * If neither hibernation nor partial power down are supported,
> * clock gating is used to save power.
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 2a7828971d05..067f2770c2ef 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -3333,7 +3333,7 @@ int dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex)
> dev_err(hsotg->dev, "enter hibernation failed.\n");
> spin_lock_irqsave(&hsotg->lock, flags);
> break;
> - case DWC2_POWER_DOWN_PARAM_NONE:
> + case DWC2_POWER_DOWN_PARAM_CLOCK_GATING:
> /*
> * If not hibernation nor partial power down are supported,
> * clock gating is used to save power.
> @@ -3701,7 +3701,7 @@ static int dwc2_hcd_hub_control(struct dwc2_hsotg *hsotg, u16 typereq,
> }
>
> if (hsotg->params.power_down ==
> - DWC2_POWER_DOWN_PARAM_NONE && hsotg->bus_suspended)
> + DWC2_POWER_DOWN_PARAM_CLOCK_GATING && hsotg->bus_suspended)
> dwc2_host_exit_clock_gating(hsotg, 0);
>
> pcgctl = dwc2_readl(hsotg, PCGCTL);
> @@ -4398,7 +4398,7 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
> /* After entering suspend, hardware is not accessible */
> clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> break;
> - case DWC2_POWER_DOWN_PARAM_NONE:
> + case DWC2_POWER_DOWN_PARAM_CLOCK_GATING:
> /*
> * If not hibernation nor partial power down are supported,
> * clock gating is used to save power.
> @@ -4482,7 +4482,7 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
> */
> set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> break;
> - case DWC2_POWER_DOWN_PARAM_NONE:
> + case DWC2_POWER_DOWN_PARAM_CLOCK_GATING:
> /*
> * If not hibernation nor partial power down are supported,
> * port resume is done using the clock gating programming flow.
> @@ -4680,7 +4680,7 @@ static int _dwc2_hcd_urb_enqueue(struct usb_hcd *hcd, struct urb *urb,
> "exit partial_power_down failed\n");
> }
>
> - if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_NONE &&
> + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_CLOCK_GATING &&
> hsotg->bus_suspended) {
> if (dwc2_is_device_mode(hsotg))
> dwc2_gadget_exit_clock_gating(hsotg, 0);
> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
> index 59e119345994..dac26410b575 100644
> --- a/drivers/usb/dwc2/params.c
> +++ b/drivers/usb/dwc2/params.c
> @@ -68,14 +68,14 @@ static void dwc2_set_his_params(struct dwc2_hsotg *hsotg)
> p->ahbcfg = GAHBCFG_HBSTLEN_INCR16 <<
> GAHBCFG_HBSTLEN_SHIFT;
> p->change_speed_quirk = true;
> - p->power_down = DWC2_POWER_DOWN_PARAM_NONE;
> + p->power_down = DWC2_POWER_DOWN_PARAM_CLOCK_GATING;
> }
>
> static void dwc2_set_s3c6400_params(struct dwc2_hsotg *hsotg)
> {
> struct dwc2_core_params *p = &hsotg->params;
>
> - p->power_down = DWC2_POWER_DOWN_PARAM_NONE;
> + p->power_down = DWC2_POWER_DOWN_PARAM_CLOCK_GATING;
> p->no_clock_gating = true;
> p->phy_utmi_width = 8;
> }
> @@ -90,7 +90,7 @@ static void dwc2_set_rk_params(struct dwc2_hsotg *hsotg)
> p->host_perio_tx_fifo_size = 256;
> p->ahbcfg = GAHBCFG_HBSTLEN_INCR16 <<
> GAHBCFG_HBSTLEN_SHIFT;
> - p->power_down = DWC2_POWER_DOWN_PARAM_NONE;
> + p->power_down = DWC2_POWER_DOWN_PARAM_CLOCK_GATING;
> }
>
> static void dwc2_set_ltq_params(struct dwc2_hsotg *hsotg)
> @@ -120,7 +120,7 @@ static void dwc2_set_amlogic_params(struct dwc2_hsotg *hsotg)
> p->phy_type = DWC2_PHY_TYPE_PARAM_UTMI;
> p->ahbcfg = GAHBCFG_HBSTLEN_INCR8 <<
> GAHBCFG_HBSTLEN_SHIFT;
> - p->power_down = DWC2_POWER_DOWN_PARAM_NONE;
> + p->power_down = DWC2_POWER_DOWN_PARAM_CLOCK_GATING;
> }
>
> static void dwc2_set_amlogic_g12a_params(struct dwc2_hsotg *hsotg)
> @@ -179,7 +179,7 @@ static void dwc2_set_stm32mp15_fsotg_params(struct dwc2_hsotg *hsotg)
> p->activate_stm_fs_transceiver = true;
> p->activate_stm_id_vb_detection = true;
> p->ahbcfg = GAHBCFG_HBSTLEN_INCR16 << GAHBCFG_HBSTLEN_SHIFT;
> - p->power_down = DWC2_POWER_DOWN_PARAM_NONE;
> + p->power_down = DWC2_POWER_DOWN_PARAM_CLOCK_GATING;
> p->host_support_fs_ls_low_power = true;
> p->host_ls_low_power_phy_clk = true;
> }
> @@ -194,7 +194,7 @@ static void dwc2_set_stm32mp15_hsotg_params(struct dwc2_hsotg *hsotg)
> p->host_nperio_tx_fifo_size = 256;
> p->host_perio_tx_fifo_size = 256;
> p->ahbcfg = GAHBCFG_HBSTLEN_INCR16 << GAHBCFG_HBSTLEN_SHIFT;
> - p->power_down = DWC2_POWER_DOWN_PARAM_NONE;
> + p->power_down = DWC2_POWER_DOWN_PARAM_CLOCK_GATING;
> p->lpm = false;
> p->lpm_clock_gating = false;
> p->besl = false;
> @@ -339,7 +339,7 @@ static void dwc2_set_param_power_down(struct dwc2_hsotg *hsotg)
> else if (hsotg->hw_params.power_optimized)
> val = DWC2_POWER_DOWN_PARAM_PARTIAL;
> else
> - val = DWC2_POWER_DOWN_PARAM_NONE;
> + val = DWC2_POWER_DOWN_PARAM_CLOCK_GATING;
>
> hsotg->params.power_down = val;
> }
> @@ -585,27 +585,27 @@ static void dwc2_check_param_power_down(struct dwc2_hsotg *hsotg)
> int param = hsotg->params.power_down;
>
> switch (param) {
> - case DWC2_POWER_DOWN_PARAM_NONE:
> + case DWC2_POWER_DOWN_PARAM_CLOCK_GATING:
> break;
> case DWC2_POWER_DOWN_PARAM_PARTIAL:
> if (hsotg->hw_params.power_optimized)
> break;
> dev_dbg(hsotg->dev,
> "Partial power down isn't supported by HW\n");
> - param = DWC2_POWER_DOWN_PARAM_NONE;
> + param = DWC2_POWER_DOWN_PARAM_CLOCK_GATING;
> break;
> case DWC2_POWER_DOWN_PARAM_HIBERNATION:
> if (hsotg->hw_params.hibernation)
> break;
> dev_dbg(hsotg->dev,
> "Hibernation isn't supported by HW\n");
> - param = DWC2_POWER_DOWN_PARAM_NONE;
> + param = DWC2_POWER_DOWN_PARAM_CLOCK_GATING;
> break;
> default:
> dev_err(hsotg->dev,
> "%s: Invalid parameter power_down=%d\n",
> __func__, param);
> - param = DWC2_POWER_DOWN_PARAM_NONE;
> + param = DWC2_POWER_DOWN_PARAM_CLOCK_GATING;
> break;
> }
>
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index c8f18f3ba9e3..7bd8fb6c1378 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -342,7 +342,7 @@ static int dwc2_driver_remove(struct platform_device *dev)
> }
>
> /* Exit clock gating when driver is removed. */
> - if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_NONE &&
> + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_CLOCK_GATING &&
> hsotg->bus_suspended) {
> if (dwc2_is_device_mode(hsotg))
> dwc2_gadget_exit_clock_gating(hsotg, 0);
>

In function dwc2_port_resume() you didn't rename the
"DWC2_POWER_DOWN_PARAM_NONE" to "DWC2_POWER_DOWN_PARAM_CLOCK_GATING" and
also didn't add a case for "DWC2_POWER_DOWN_PARAM_NONE".

In any case when we tested the patchset we encountered a problem when
using an external HUB connection to root HUB.

If there is no device connected to the external HUB bus will be
suspended by the autosuspend. If this situation by connecting any device
to the external hub ports we don't see dwc2_port_resume() function call
and device remains not functional.

Could you please perform the same test on your side and let us know the
results?

Regards,
Artur

Next message: Chao Yu: "Re: [PATCH] f2fs: Kconfig: clean up config options about compression"
Previous message: chenguanyou: "[PATCH v2] hungtask: add filter kthread"
In reply to: Greg KH: "Re: [PATCH 1/2] usb: dwc2: rename DWC2_POWER_DOWN_PARAM_NONE state"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]